In the C++ bag of tricks: scoped locks (opens in new tab)

(blog.skanev.org)

43 pointss_kanev12y ago51 comments

51 comments

The real advantage of a scoped lock is that it promises to release the mutex, no matter how the scope is exited. If this LOCKED macro is used and an exception is thrown from within the block, that mutex now stays locked.

> The only gotcha is, there is some overhead involved in this approach. The class instance takes up some space on the stack (several bytes) for every lock acquisition.

Not so fast. A decent compiler will eliminate the overhead and not actually allocate stack space for the lock. Here's an example using one of the lock guards in C++:

    struct foo {
      int var;
      std::mutex m;
    };
    
    void process1(foo& f) {
      std::lock_guard<std::mutex> lock(f.m);
      ++f.var;  
    }

    void process2(foo& f) {
      f.m.lock();
      ++f.var;  
      f.m.unlock();
    }

GCC 4.8.1 generates identical code for process1 and process2.

lbrandy12y ago

Indeed. Sorry OP, your solution:

(1) avoids best practice (RAII) using...

(2) a flawed rationale (ignoring the existence of compiler optimizations)...

(3) without measurement to demonstrate the supposed flaw in the best practice (which would have surfaced the flawed rationale)...

(4) and therefore implements a micro-optimization with no actual win...

(5) and introduces a bug in the process (it's not exception safe).

s_kanevOP12y ago

OP here. Totally agree. TIL about RAII. That particular codebase doesn't use c++ exceptions, but still not a reason for a roll-your-own solution if something as efficient is in the standard library.

1 more reply

stinos12y ago

This. For at least three typical 'best practice' reasons. I know there are times best practices can be nicely ignored, but this is not one of those occasions. On the contrary. Let me lay them out again:

- these are the days of C++11 where scoped locks are readily available

- premature optimization

- macros are evil

pjmlp12y ago

One of the reasons C++ gets bad reputation, is those Cisms that people insist on writing, when the language offers much better alternatives.

1 more reply

twoodfin12y ago

Yeah, they even admit that their macro compiles to identical code as the original RAII version. Where's the "overhead"?

As a general rule, if your C++ compiler can statically see the layout of your structures and the implementation of your functions, it will produce code as efficient as inlining everything yourself. Sufficiently complex functions make the benefits of inlining a judgment call on the compiler's part, but for something as simple as an RAII scoped lock, the abstraction is free.

infogulch12y ago

Wait... if it's identical, how can it be safe against exceptions? `process2` isn't.

saurik12y ago

In addition to twoodfin's answer, I will point out that even if the code could throw, the code for the function would still be identical for when a throw did not occur (as in, we would see identical output with the addition of some code that would seemingly never be used or executed), on "good architectures", due to zero-cost exceptions (which only incur overhead during unwind; if no exception is thrown the code executed is exactly identical to if exceptions were not possible).

twoodfin12y ago

gcc can statically determine that incrementing f.var will not throw.

fighterjet12y ago

I fail to see the point of using a macro (ew) to accomplish something that's included, with several different implementations, in the c++11 standard library

taway201212y ago

Don't do this. The whole thing gave off a bad smell on first reading so I would never have done this.

Thank you vinkelhake for nailing the the real problem (exceptions) and bothering to check whether the stack overhead really exists.

Just wanted to chime in with a comment because HN doesn't show vote counts, so future readers will "see" my upvote on vinkelhake's comment.

jacobparker12y ago

Exception issues aside, an odd definition of "scoped". A very non-C++ solution. Use RAII and lock_guard (if possible) as vinkelhake demonstrates. Consider,

    LOCKED(lk_var) {
        return 42; // whoops
    }

VikingCoder12y ago

You're also consuming the variable name i, which everyone else is probably using.

Speaking of which, this means you can't have one of your locks scoped inside another one.

cjensen12y ago

Solvable by creating a Preprocessor macro which creates a unique and complex-named identifier.

  #define LOCKED(lk) for(int UNIQUE_ID=0; UNIQUE_ID<1 && !lk_lock(lk); lk_unlock(lk), UNIQUE_ID++)

Use __LINE__ to help make the id unique.

VikingCoder12y ago

Then you still can't forget and do this:

    lock(lk_var) lock(lk_var2)
    {
        lk_var = lk_var2;
    }

Or this:

    lock(lk_var) { lock(lk_var2 { lk_var = lk_var2; } }

Using the pre-processor to solve a regular, every day, hum-drum, solved-a-billion-times RAII problem is bad news.

JoeAltmaier12y ago

SUre you could. The{} make it work.

VikingCoder12y ago

EDIT: I sit corrected. My post is wrong.

No, you can't, and no they don't.

    lock(lk_var) {
        lock(lk_var2) {
            var = var2;
        }
    }

That expands to:

    for(int i=0; (i < 1) && !lk_lock(lk_var); lk_unlock(lk_var), i++)
    {
        for(int i=0; (i < 1) && !lk_lock(lk_var2); lk_unlock(lk_var2), i++)
        {
            var = var2;
        }
    }

You end up with an i scoped inside another i.

1 more reply

steveklabnik12y ago

This kind of thing is one area I'm excited to see work in Rust: you can use Rust's safety guarantees to ensure that all access goes through the lock.

JoeAltmaier12y ago

Works, but requires {} for scope. The local-object version can be scoped however you like. It optimizes away just fine, similar to the for loop.

ef47d35620c112y ago

C++11 has scoped locks built-in, doesn't it?

steve-howard12y ago

It offers a variety of them, simplest of which is lock_guard:

http://en.cppreference.com/w/cpp/thread/lock_guard

infogulch12y ago

What about:

     #define LOCKED(lk) for(int i=lk_lock(lk); i == 0; lk_unlock(lk), i++)

Probably better to test for 0 exactly, in case it fails. You don't want it to unlock twice just because it returns -1.

eps12y ago

This is so wrong on so many levels.

infogulch12y ago

It's not any worse than what was suggested in the linked article...

8_hours_ago12y ago

A very important gotcha with this approach is that any variable named "i" will be reset to 0 inside of the lock, and modifying "i" inside of the lock can cause an infinite loop.

This can be mitigated by generating a unique variable name with the C preprocessor: http://stackoverflow.com/questions/1132751/how-can-i-generat...

That being said, it's much better to use a standard method of scoped locking instead of trying to reinvent the wheel.

ambrop712y ago

I learned about the for loop hack from avr-libc[1][2]. It even does some preprocessor abuse to implement different kinds of atomic and non-atomic sections.

[1] http://www.nongnu.org/avr-libc/user-manual/group__util__atom...

[2] http://svn.savannah.nongnu.org/viewvc/trunk/avr-libc/include...

mojuba12y ago

Wouldn't this be exception-safe, provided lock_iter defines a ctor, dtor, operator bool() and operator ++()?

    #define LOCKED(lk) for (lock_iter i = lk; i; i++)

(Edit: I thought lock_iter could be an empty class but no, it can not, so I edited my post. lock_iter will have an internal flag that should be optimized away following the same logic int i is optimized in the OP's version)

ska12y ago

People have already pointed out the specific problems with this approach so I won't repeat them.

It's worth reiterating, though: If you are writing c++ these days and find yourself reaching for a preprocessor macro, I think that's a good sign you should think really carefully about what you are actually trying to do. Chances are really good there is a (much) better way.

dllthomas12y ago

You can do some of this with the cleanup attribute in GCC, although it doesn't trigger in the case of a longjmp.

khc12y ago

if you can rely on gcc, cleanup attribute is probably the best you can do. The solution in original post doesn't cope with "break", "continue", "return" (and of course, "goto", but I don't remember if cleanup attribute handles that either).

Looks like their project already uses C++ though, so why don't they just use C++ for this is beyond me.

dllthomas12y ago

Agreed.

dllthomas12y ago

Actually, it looks like C++ destructors don't run on longjmp either, though that's less of an issue since C++ actually has real exceptions.

mpyne12y ago

longjmp is what the library and standards writers refer to as a "non-local goto" so it's kind of to be expected that it would manage to bypass destructors (I'm not sure how you'd unwind the stack enough to ensure that you've destructed everything created since the corresponding setjmp).

1 more reply

cbsmith12y ago

Boost.Scopexit addresses this point pretty well I think: http://www.boost.org/doc/libs/1_55_0/libs/scope_exit/doc/htm...

zeroonetwothree12y ago

folly::Synchronized implements this in a correct way (and also prevents access without acquiring the lock):

SYNCHRONIZED(data) { // modify data }

https://github.com/facebook/folly/blob/master/folly/Synchron...

blt12y ago

This is not exception-safe like scoped locks are.

j / k navigate · click thread line to collapse

51 comments

vinkelhake12y ago

> The only gotcha is, there is some overhead involved in this approach. The class instance takes up some space on the stack (several bytes) for every lock acquisition.

Not so fast. A decent compiler will eliminate the overhead and not actually allocate stack space for the lock. Here's an example using one of the lock guards in C++:

    struct foo {
      int var;
      std::mutex m;
    };
    
    void process1(foo& f) {
      std::lock_guard<std::mutex> lock(f.m);
      ++f.var;  
    }

    void process2(foo& f) {
      f.m.lock();
      ++f.var;  
      f.m.unlock();
    }

GCC 4.8.1 generates identical code for process1 and process2.

lbrandy12y ago

Indeed. Sorry OP, your solution:

(1) avoids best practice (RAII) using...

(2) a flawed rationale (ignoring the existence of compiler optimizations)...

(3) without measurement to demonstrate the supposed flaw in the best practice (which would have surfaced the flawed rationale)...

(4) and therefore implements a micro-optimization with no actual win...

(5) and introduces a bug in the process (it's not exception safe).

s_kanevOP12y ago

OP here. Totally agree. TIL about RAII. That particular codebase doesn't use c++ exceptions, but still not a reason for a roll-your-own solution if something as efficient is in the standard library.

1 more reply

stinos12y ago

- these are the days of C++11 where scoped locks are readily available

- premature optimization

- macros are evil

pjmlp12y ago

One of the reasons C++ gets bad reputation, is those Cisms that people insist on writing, when the language offers much better alternatives.

1 more reply

twoodfin12y ago

Yeah, they even admit that their macro compiles to identical code as the original RAII version. Where's the "overhead"?

infogulch12y ago

Wait... if it's identical, how can it be safe against exceptions? `process2` isn't.

saurik12y ago

twoodfin12y ago

gcc can statically determine that incrementing f.var will not throw.

fighterjet12y ago

I fail to see the point of using a macro (ew) to accomplish something that's included, with several different implementations, in the c++11 standard library

taway201212y ago

Don't do this. The whole thing gave off a bad smell on first reading so I would never have done this.

Thank you vinkelhake for nailing the the real problem (exceptions) and bothering to check whether the stack overhead really exists.

Just wanted to chime in with a comment because HN doesn't show vote counts, so future readers will "see" my upvote on vinkelhake's comment.

jacobparker12y ago

Exception issues aside, an odd definition of "scoped". A very non-C++ solution. Use RAII and lock_guard (if possible) as vinkelhake demonstrates. Consider,

    LOCKED(lk_var) {
        return 42; // whoops
    }

VikingCoder12y ago

You're also consuming the variable name i, which everyone else is probably using.

Speaking of which, this means you can't have one of your locks scoped inside another one.

cjensen12y ago

Solvable by creating a Preprocessor macro which creates a unique and complex-named identifier.

  #define LOCKED(lk) for(int UNIQUE_ID=0; UNIQUE_ID<1 && !lk_lock(lk); lk_unlock(lk), UNIQUE_ID++)

Use __LINE__ to help make the id unique.

VikingCoder12y ago

Then you still can't forget and do this:

    lock(lk_var) lock(lk_var2)
    {
        lk_var = lk_var2;
    }

Or this:

    lock(lk_var) { lock(lk_var2 { lk_var = lk_var2; } }

Using the pre-processor to solve a regular, every day, hum-drum, solved-a-billion-times RAII problem is bad news.

JoeAltmaier12y ago

SUre you could. The{} make it work.

VikingCoder12y ago

EDIT: I sit corrected. My post is wrong.

No, you can't, and no they don't.

    lock(lk_var) {
        lock(lk_var2) {
            var = var2;
        }
    }

That expands to:

    for(int i=0; (i < 1) && !lk_lock(lk_var); lk_unlock(lk_var), i++)
    {
        for(int i=0; (i < 1) && !lk_lock(lk_var2); lk_unlock(lk_var2), i++)
        {
            var = var2;
        }
    }

You end up with an i scoped inside another i.

1 more reply

steveklabnik12y ago

This kind of thing is one area I'm excited to see work in Rust: you can use Rust's safety guarantees to ensure that all access goes through the lock.

JoeAltmaier12y ago

Works, but requires {} for scope. The local-object version can be scoped however you like. It optimizes away just fine, similar to the for loop.

ef47d35620c112y ago

C++11 has scoped locks built-in, doesn't it?

steve-howard12y ago

It offers a variety of them, simplest of which is lock_guard:

http://en.cppreference.com/w/cpp/thread/lock_guard

infogulch12y ago

What about:

     #define LOCKED(lk) for(int i=lk_lock(lk); i == 0; lk_unlock(lk), i++)

Probably better to test for 0 exactly, in case it fails. You don't want it to unlock twice just because it returns -1.

eps12y ago

This is so wrong on so many levels.

infogulch12y ago

It's not any worse than what was suggested in the linked article...

8_hours_ago12y ago

A very important gotcha with this approach is that any variable named "i" will be reset to 0 inside of the lock, and modifying "i" inside of the lock can cause an infinite loop.

This can be mitigated by generating a unique variable name with the C preprocessor: http://stackoverflow.com/questions/1132751/how-can-i-generat...

That being said, it's much better to use a standard method of scoped locking instead of trying to reinvent the wheel.

ambrop712y ago

I learned about the for loop hack from avr-libc[1][2]. It even does some preprocessor abuse to implement different kinds of atomic and non-atomic sections.

[1] http://www.nongnu.org/avr-libc/user-manual/group__util__atom...

[2] http://svn.savannah.nongnu.org/viewvc/trunk/avr-libc/include...

mojuba12y ago

Wouldn't this be exception-safe, provided lock_iter defines a ctor, dtor, operator bool() and operator ++()?

    #define LOCKED(lk) for (lock_iter i = lk; i; i++)

ska12y ago

People have already pointed out the specific problems with this approach so I won't repeat them.

dllthomas12y ago

You can do some of this with the cleanup attribute in GCC, although it doesn't trigger in the case of a longjmp.

khc12y ago

Looks like their project already uses C++ though, so why don't they just use C++ for this is beyond me.

dllthomas12y ago

Agreed.

dllthomas12y ago

Actually, it looks like C++ destructors don't run on longjmp either, though that's less of an issue since C++ actually has real exceptions.

mpyne12y ago

1 more reply

cbsmith12y ago

Boost.Scopexit addresses this point pretty well I think: http://www.boost.org/doc/libs/1_55_0/libs/scope_exit/doc/htm...

zeroonetwothree12y ago

folly::Synchronized implements this in a correct way (and also prevents access without acquiring the lock):

SYNCHRONIZED(data) { // modify data }

https://github.com/facebook/folly/blob/master/folly/Synchron...

blt12y ago

This is not exception-safe like scoped locks are.

j / k navigate · click thread line to collapse