1

Possible Duplicate:
When to use volatile with multi threading?

I have two threads referencing the same boost::shared_ptr:

boost::shared_ptr<Widget> shared;

On thread is spinning, waiting for the other thread to reset the boost::shared_ptr:

while(shared)
   boost::thread::yield();

And at some point the other thread will call:

shared.reset();

My question is whether or not I need to declare the shared pointer as volatile to prevent the compiler from optimizing the call to shared.operator bool() out of the loop and never detecting the change? I know that if I were simply looping on a variable, waiting for it to reach 0 I would need volatile, but I'm not sure if boost::shared_ptr is implemented in such a way that it is not necessary here.

EDIT: I'm fully aware that condition variables can be used to solve this problem in a different way. But in this case, the busy loop is very uncommon and contending for the lock on the condition variable is an overhead we would rather not incur.

Community
  • 1
  • 1
JaredC
  • 5,150
  • 1
  • 20
  • 45
  • 7
    Oh boy. Here we go... – John Dibling Jan 11 '11 at 20:51
  • 1
    Please, don't. Stop that now. There is a huge number of reasons to not do what you are trying to do. Then, take a good read at http://www.boost.org/doc/libs/1_45_0/doc/html/thread/synchronization.html#thread.synchronization.condvar_ref – Juliano Jan 11 '11 at 20:53
  • @JaredC: Please take a look at [this][1], [this][2] and [this][3] for more information about `volatile` [1]: http://stackoverflow.com/questions/4168735/is-this-rule-about-volatile-usage-strict [2]: http://software.intel.com/en-us/blogs/2007/11/30/volatile-almost-useless-for-multi-threaded-programming/ [3]: http://stackoverflow.com/questions/4136900/what-rules-does-compiler-have-to-follow-when-dealing-with-volatile-memory-locatio – John Dibling Jan 11 '11 at 20:58
  • 1
    @JaredC: And this, probably first of all: http://stackoverflow.com/questions/4557979/when-to-use-volatile-with-multi-threading – John Dibling Jan 11 '11 at 21:02
  • "condition variable is an overhead" have you measured it? It looks like premature optimization. – Yakov Galka Jan 11 '11 at 21:12
  • 1
    @John: In fact the answer to the question of that last link ("`volatile` is useless") seems to answer this question as well. – sbi Jan 11 '11 at 21:14
  • @ybung We are in the process of removing locks from this performance sensitive section of code, so we have determined their overhead. This code gets executed 10000+ times per second, so we're trying to streamline it. – JaredC Jan 11 '11 at 21:17
  • 3
    @JaredC: Given your motivations, please bear in mind that the true purpose of `volatile` is to suppress optimizations. This seems to be the exact opposite of the reason you may be trying to use it. – John Dibling Jan 11 '11 at 21:29
  • 1
    @John: Thanks for the links and input. I am going to review those articles (and their links (and their links)) to completely understand volatile. Sorry to post on a such a sore topic. – JaredC Jan 11 '11 at 21:31
  • 1
    @JaredC: Don't apologize. This is just my personal soapbox topic. It is an excellent question. In fact your question has prompted us (in C++ chat) to write a FAQ on the subject. – John Dibling Jan 11 '11 at 21:38

2 Answers2

7

Rant 1:

That code probably won't do what you think it will. When you write code like that, you're introducing a data race into your code. This is almost certainly a bug that will result in your program non-deterministically failing.

Data structures (including shared_ptr) are generally not meant to be accessed concurrently. Do not modify the same structure at the same time in more than one thread. That could corrupt the structure. Do not modify it in one thread and read it in another thread. The reader could see inconsistent data. Probably multiple threads can read it at the same time.

If you think you really want to do some of the above, find out if the data structure allows some of these behaviors in a section probably titled "Thread Safety." If it does allow them, take a second look at whether your performance really needs this, and then use it. (The documentation on shared_ptr does NOT allow what you're doing.)

Rant 2:

Now, for a higher-level concern, you probably shouldn't be doing thread synchronization by waiting for a pointer to be set to NULL. Really, look at condition variables, barriers, or futures as a way of getting one thread to wait until another is finished with something. It's a nicer interface, and whoever looks at your code next (that includes you in 6 months) will thank you.

I know you're concerned about the performance cost of real synchronization. Don't worry about this. It'll be fine. If you're worried about lock contention, use barriers or futures, which don't have a big shared lock to contend for.

Caveat: there is a time for writing code that avoids locks at all cost. But unless you're looking at profiler data that says your synch ops are too slow for your target workload, this isn't the time.

Rant 3:

I hope that shared in your example is global. Otherwise, you have multiple threads with local references to the same shared_ptr that points to the real object you're interested in. It kind of defeats the purpose of having a reference-counted pointer. Just please tell me it's global.

Karmastan
  • 5,618
  • 18
  • 24
  • Rant3: shared is a global variable, yes. Rant2: We are at the point of optimization, which is why I am removing locks in this code section in the first place. Rant1: This is the most valid argument I've heard for not doing this, thanks. – JaredC Jan 11 '11 at 21:55
4

What you actually should do is to use condition variables. Busy waits are evil.

Edit: also depending on your task, futures may be even cleaner way to achieve what you want.

Yakov Galka
  • 70,775
  • 16
  • 139
  • 220
  • I agree usually, but in this case the loop should almost never get executed, so we've decided that busy looping is better than contending for the lock. – JaredC Jan 11 '11 at 20:54
  • Then you can easily check once before acquiring the lock, then you almost never will acquire the lock. Still it's not an excuse to busy wait. Without lock you can't guarantee that shared remains empty after the loop. – Yakov Galka Jan 11 '11 at 20:56
  • @JaredC: If the loop almost never gets executed then the lock will almost never be contended. – James McNellis Jan 11 '11 at 20:57
  • 2
    JaredC: Busy looping is never ( * ) better than using a monitor, periode. (Note: ( * ) in some cases, it may actually be better, namely, in kernel code in non-battery-powered embedded systems, but that most probably is not your case). – Juliano Jan 11 '11 at 20:58
  • @ybung, checking once before looping would still require the thread calling reset() to acquire the lock always. – JaredC Jan 11 '11 at 21:00
  • @JaredC: no. you *always* call condVar.notify_all() without a lock. – Yakov Galka Jan 11 '11 at 21:03
  • 1
    @ybung, But then there is a race condition that the the waiting thread might not actually be waiting YET, but have already seen its condition as false. Then, the reset() call happens and the notify_all() wakes up exactly no one. Am I missing something? – JaredC Jan 11 '11 at 21:07
  • @JaredC: oops, my mistake. You're right here. – Yakov Galka Jan 11 '11 at 21:10