Primer on locks and condition variables now on chromium.org

[Fred Akalin]

Hello everyone,

With permission, and on siggi@'s suggestion, I took Mike Burrows' excellent (internal-only) document on mutexes and condition variables and put an adapted version on our sites page.

We shouldn't be using these low-level synchronization primitives very often, but when we need to, this document explains the best practices and pitfalls of doing so.

There are some TODOs in the document: one to get the anchor links working, and the rest to verify some assertions about efficiency and functionality.  Perhaps someone with more sites-fu can fix the first, and someone with deeper knowledge of the behavior of the OS-specific primitives we use can fill in the rest.

Let me know if you have any questions, comments, or suggestions!

- Fred

[William Chan (陈智昌)]

Hey Fred,

While I support this in general, there are a bunch of controversial things / google3isms. For an example of controversial, I think the recommended commenting practice may be debatable. I am all for it, but I don't know if others are. So I'm concerned about saying it's "strongly recommended", especially since most (all?) of our code doesn't follow this convention. Also, there are many places in the document that reference Mutex instead of Lock. We also don't have CondVars.

Cheers.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Ryan Sleevi]

On Mon, Mar 12, 2012 at 5:16 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

We also don't have CondVars.

A naming typo. We've had them since the beginning - just named ConditionVariable.

http://src.chromium.org/viewvc/chrome/trunk/src/base/synchronization/condition_variable.h?revision=HEAD&view=markup 

[William Chan (陈智昌)]

No, they're not a naming typo. They're a google3 class. Note the member functions names. They are 2 different classes that implement the general condition variable paradigm.

[Joao da Silva]

That was a great read, thanks for sharing. I'd like to know what's recommended in chromium regarding this:

"""Why not always use AutoLock?

Critical section boundaries do not always make it convenient to use AutoLock. Even when they do, programmers should be aware that using AutoLock is not always an improvement.

Like scoped_ptr, AutoLock introduces a tradeoff in readability. It makes it clear that the lock is released, but because the Release() becomes implicit, it may become less clear when it is released, especially in complex routines with long blocks."""

I actually find AutoLock and scoped_ptr way easier to read and understand, and way more robust regarding future edits around the code in scope. So what's the encouraged style for chromium?

- Joao

[Fred Akalin]

My mistake.  I scrubbed the member functions but not the class name itself. *sigh* I'll fix it.

[Fred Akalin]

On Mon, Mar 12, 2012 at 5:16 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

Hey Fred,

While I support this in general, there are a bunch of controversial things / google3isms. For an example of controversial, I think the recommended commenting practice may be debatable. I am all for it, but I don't know if others are. So I'm concerned about saying it's "strongly recommended", especially since most (all?) of our code doesn't follow this convention. Also, there are many places in the document that reference Mutex instead of Lock. We also don't have CondVars.

 

Okay, I'll dial down some of the recommendations (strongly recommended -> recommended).  I changed the class name from Mutex to Lock, but I think it's okay if the document uses "mutex" and "lock" interchangeably.  I'll also fix the CondVar name.

Note that any chromium committer can edit the document, so if you find other mistakes, feel free to fix them!

[Fred Akalin]

On Mon, Mar 12, 2012 at 5:33 PM, Joao da Silva <joaod...@google.com> wrote:

That was a great read, thanks for sharing. I'd like to know what's recommended in chromium regarding this:

"""Why not always use AutoLock?

Critical section boundaries do not always make it convenient to use AutoLock. Even when they do, programmers should be aware that using AutoLock is not always an improvement.

Like scoped_ptr, AutoLock introduces a tradeoff in readability. It makes it clear that the lock is released, but because the Release() becomes implicit, it may become less clear when it is released, especially in complex routines with long blocks."""

I actually find AutoLock and scoped_ptr way easier to read and understand, and way more robust regarding future edits around the code in scope. So what's the encouraged style for chromium?

I wondered about that, too.  I haven't run into a case where manual locking/unlocking would have been clearer, especially with AutoUnlock.  I'll change that section.

[William Chan (陈智昌)]

While you're at it, remember that there is no Await() either in Lock. Also there's no ReaderAcquire(). Also, in Chromium we do std::deque instead of "using std::deque;" (ew!!!).

[Antoine Labour]

"Most programmers use mutexes as their main technique, but message passing when it is particularly appropriate."

I think in chrome code, we tend to do the opposite. Use "messages" (PostTask) in doubt, but mutexes locally when it makes sense. Worth noting IMO.

Antoine

[William Chan (陈智昌)]

This is yet another m3b'ism. He hates RAII. I think he would prefer we use C instead of C++. I strongly recommend using AutoLock.

This is yet another reason why I'd be cautious about all the recommendations which haven't been discussed here. Internally, this doc is in m3b's home directory, and not an official document (although widely referenced, because m3b has many solid recommendations and is a concurrency badass). I recommend selectively copying parts rather than doing a wholesale copy and post-trim/edit. It's a wonderful read for anyone who cares about concurrency, but be cautious about some of the recommendations here, which are also debated internally, since Googlers are very contentious about style.

Note, we only have 2 thread priority levels, so the priority section is largely irrelevant, and I'd actually simply rather the vast majority Chromium engineers not think about thread priorities at all. Honestly, I'd rather people try to avoid Locks. Chromium style is to keep most code single threaded and use message passing. I'd rather be consistent and not encourage people to use Locks.

What's the motivation here for this document? While an awesome concurrency doc, I'd rather we spend more time improving documentation about our Chromium patterns. And perhaps advocating using the thread annotation style commenting with our Locks. Just my two cents.

On Mon, Mar 12, 2012 at 5:33 PM, Joao da Silva <joaod...@google.com> wrote:

[Scott Hess]

On Mon, Mar 12, 2012 at 5:47 PM, Antoine Labour <pi...@google.com> wrote:
> "Most programmers use mutexes as their main technique, but message passing
> when it is particularly appropriate."
>
> I think in chrome code, we tend to do the opposite. Use "messages"
> (PostTask) in doubt, but mutexes locally when it makes sense. Worth noting
> IMO.

I think it's worthwhile for the top of the document to suggest that
you shouldn't use explicit locking in Chromium except as a last resort
where you've definitively demonstrated that PastTask and kin simply
won't do the job. My general feeling is that for most developers,
once they're trying to decide whether they need a lock or not, they've
already lost the battle, and no amount of documenting things is going
to salvage the situation.

-scott

[Fred Akalin]

Okay, I think I addressed everyone's comments with the latest version:
https://sites.google.com/a/chromium.org/dev/developers/lock-and-condition-variable
.

(Also, since it's not obvious how to edit the document, you have to
scroll all the way down and click the 'Sign In' link way at the bottom
before the 'Edit' controls show up.)

> This is yet another m3b'ism. He hates RAII. I think he would prefer we use C
> instead of C++. I strongly recommend using AutoLock.

Done.

> This is yet another reason why I'd be cautious about all the recommendations
> which haven't been discussed here. Internally, this doc is in m3b's home
> directory, and not an official document (although widely referenced, because
> m3b has many solid recommendations and is a concurrency badass). I recommend
> selectively copying parts rather than doing a wholesale copy and
> post-trim/edit. It's a wonderful read for anyone who cares about
> concurrency, but be cautious about some of the recommendations here, which
> are also debated internally, since Googlers are very contentious about
> style.

This document is actually already pretty cut-down from the original.
But I guess I wasn't ruthless enough in trimming it.

> Note, we only have 2 thread priority levels, so the priority section is
> largely irrelevant, and I'd actually simply rather the vast majority
> Chromium engineers not think about thread priorities at all. Honestly, I'd
> rather people try to avoid Locks. Chromium style is to keep most code single
> threaded and use message passing. I'd rather be consistent and not encourage
> people to use Locks.

Removed, and posted a warning at the top.

> What's the motivation here for this document? While an awesome concurrency
> doc, I'd rather we spend more time improving documentation about our
> Chromium patterns. And perhaps advocating using the thread annotation style
> commenting with our Locks. Just my two cents.

Basically, this came out of
http://code.google.com/p/chromium/issues/detail?id=117469 and the
underlying bug, which would have likely been prevented by the
recommendations in the document. Even though 99% of the time we
should be using TaskRunner/MessageLoops, the other 1% of the time it's
useful to have a document which outlines the best practices and the
pitfalls of using locks/CVs.

On Mon, Mar 12, 2012 at 5:38 PM, William Chan (陈智昌)
<will...@chromium.org> wrote:
> While you're at it, remember that there is no Await() either in Lock. Also
> there's no ReaderAcquire(). Also, in Chromium we do std::deque instead of
> "using std::deque;" (ew!!!).

Done.

> I think in chrome code, we tend to do the opposite. Use "messages"
> (PostTask) in doubt, but mutexes locally when it makes sense. Worth noting
> IMO.

Done.

On Mon, Mar 12, 2012 at 5:57 PM, Scott Hess <sh...@chromium.org> wrote:
> I think it's worthwhile for the top of the document to suggest that
> you shouldn't use explicit locking in Chromium except as a last resort
> where you've definitively demonstrated that PastTask and kin simply
> won't do the job.  My general feeling is that for most developers,
> once they're trying to decide whether they need a lock or not, they've
> already lost the battle, and no amount of documenting things is going
> to salvage the situation.

Done.

[Darin Fisher]

Our main document on threading is here:

https://sites.google.com/a/chromium.org/dev/developers/design-documents/threading

Perhaps this new document should be linked to from that one?  Perhaps this new document also belongs in the design docs section?

I don't think our site is terribly well organized, but you seem to have inserted this into a coding style section.  I think I wouldn't classify the new document as a coding style document.  Instead, it seems more like a set of guidelines for how to use locks and cvars properly.  It seems like it belongs in a different section :-/

-Darin

[Fred Akalin]

On Mon, Mar 12, 2012 at 6:47 PM, Darin Fisher <da...@chromium.org> wrote:
> Our main document on threading is here:
> https://sites.google.com/a/chromium.org/dev/developers/design-documents/threading
>
> Perhaps this new document should be linked to from that one?  Perhaps this
> new document also belongs in the design docs section?
>
> I don't think our site is terribly well organized, but you seem to have
> inserted this into a coding style section.  I think I wouldn't classify the
> new document as a coding style document.  Instead, it seems more like a set
> of guidelines for how to use locks and cvars properly.  It seems like it
> belongs in a different section :-/

Hmm. Perhaps it doesn't belong under 'coding style', but maybe it
still belongs under 'Practices'? Maybe the threading document belongs
there, too; I wouldn't have thought to look in design documents for
nitty-gritty technical stuff like using threads and such.

[Darin Fisher]

Yeah, neither document seems like a "design document" to me.

The "Contributing code" section seems a bit too high level to be the place were we go into detail about locks and cvars.  I also think the sub-bullets under "Coding style" could just be links from the "Coding style" document.

I'm not sure I have a good suggestion for where these threading documents should live.  I could also imagine these documents under the "How-tos" section.  These documents are really about how to write good multi-threaded code in Chromium.

-Darin

[Sigurður Ásgeirsson]

On Mon, Mar 12, 2012 at 20:51, William Chan (陈智昌) <will...@chromium.org> wrote:

This is yet another m3b'ism. He hates RAII. I think he would prefer we use C instead of C++. I strongly recommend using AutoLock.

This is yet another reason why I'd be cautious about all the recommendations which haven't been discussed here. Internally, this doc is in m3b's home directory, and not an official document (although widely referenced, because m3b has many solid recommendations and is a concurrency badass). I recommend selectively copying parts rather than doing a wholesale copy and post-trim/edit. It's a wonderful read for anyone who cares about concurrency, but be cautious about some of the recommendations here, which are also debated internally, since Googlers are very contentious about style.

Note, we only have 2 thread priority levels, so the priority section is largely irrelevant, and I'd actually simply rather the vast majority Chromium engineers not think about thread priorities at all. Honestly, I'd rather people try to avoid Locks. Chromium style is to keep most code single threaded and use message passing. I'd rather be consistent and not encourage people to use Locks.

What's the motivation here for this document? While an awesome concurrency doc, I'd rather we spend more time improving documentation about our Chromium patterns. And perhaps advocating using the thread annotation style commenting with our Locks. Just my two cents.

The motivation here is Fred's fix for http://crbug.com/117469, which I encountered and reported. IMHO m3b's original document contains the most concise, actionable description of Mutex/CV usage that I've ever encountered, which is why I wondered if we could re-publish it.

I do agree that with the general sentiment that locking should be avoided wherever possible :).

[Fred Akalin]

On Mon, Mar 12, 2012 at 7:02 PM, Darin Fisher <da...@chromium.org> wrote:
> Yeah, neither document seems like a "design document" to me.
>
> The "Contributing code" section seems a bit too high level to be the place
> were we go into detail about locks and cvars.  I also think the sub-bullets
> under "Coding style" could just be links from the "Coding style" document.
>
> I'm not sure I have a good suggestion for where these threading documents
> should live.  I could also imagine these documents under the "How-tos"
> section.  These documents are really about how to write good multi-threaded
> code in Chromium.

I think the closest section would be the 'Development Tips & Tricks'
section, although I think if we were to move the threading/mutex docs
there, we'd have to rename it something like 'Development Guides'.
What do you think?

[Darin Fisher]

I agree, that works.

-Darin

[Gregg Tavares (勤)]

On Mon, Mar 12, 2012 at 5:02 PM, Fred Akalin <aka...@chromium.org> wrote:

Hello everyone,

With permission, and on siggi@'s suggestion, I took Mike Burrows' excellent (internal-only) document on mutexes and condition variables and put an

this link

 

adapted version

Gets me "page not found" and nothing shows up in the sitemap that I could find. 

 

 

n our sites page.

We shouldn't be using these low-level synchronization primitives very often, but when we need to, this document explains the best practices and pitfalls of doing so.

There are some TODOs in the document: one to get the anchor links working, and the rest to verify some assertions about efficiency and functionality.  Perhaps someone with more sites-fu can fix the first, and someone with deeper knowledge of the behavior of the OS-specific primitives we use can fill in the rest.

Let me know if you have any questions, comments, or suggestions!

- Fred

[Fred Akalin]

The original URL changed since I renamed condvar to conditionvariable.
It's at https://sites.google.com/a/chromium.org/dev/developers/lock-and-condition-variable,
which is linked from http://dev.chromium.org/developers .

[Fred Akalin]

On Tue, Mar 13, 2012 at 9:19 PM, Darin Fisher <da...@chromium.org> wrote:

I agree, that works.

-Darin

Done!  I also added the document about subtle threading bugs and how to avoid them.

[Carlos Pizano]

Condition variables are nearly impossible to implement correctly on
Windows XP or earlier [1]. Our implementation I believe is correct but
_very_ slow. Whenever you use a CV you are hurting our Windows XP
users. So please don't.

Use locking and message passing. That is the chrome way.

-cpu

[1] Vista has a native CV but who knows how good it is/

[Darin Fisher]

Plus a lot of times people just want to wait on a boolean.  In such cases, please use WaitableEvent instead.

[Jonathan Dixon]

This is useful background. I went ahead and added both these warnings to the general text at the top of https://sites.google.com/a/chromium.org/dev/developers/lock-and-condition-variable, hope that's OK Fred :)

--

[Fred Akalin]

Of course!  Everyone, feel free to expand and improve the document.

[Lambros Lambrou]

On Fri, Mar 23, 2012 at 10:20 AM, Fred Akalin <aka...@chromium.org> wrote:

Of course!  Everyone, feel free to expand and improve the document.

I had a look at this document, it's really good (one of the best I've read) but it left me with a couple of unanswered questions, and I'm hoping some experts reading this can set the record straight.

From the document: "Variables accessed by more than one thread and written by at least one thread should be protected consistently by a Lock."

Does this apply to booleans?  Is there a consensus on how booleans should be treated in Chrome code?  Do they need full locking?  Declaring as volatile OK?  Or don't bother?  My own understanding is that booleans do need some kind of locking, even though booleans are atomic, because there is a possibility of a change made in one thread not becoming visible in another thread.  Is this actually true on systems we care about?

My second question is about this assertion from the document: "An exception to this rule is that fields may be initialized in constructors without a mutex, since no other thread should be able to reference the memory at that time."

I seem to remember reading something a long time ago that contradicted this assertion (and claimed it to be a common myth).  Unfortunately, I can't remember where I read this, and haven't been able to find anything I'd trust as definitive.  The claim I read was (IIRC): if you allocate a new object, initialize one of the fields (in initialization list or ctor), then access the object from another thread without using a memory barrier, the other thread might see the "default" value of the field, and not the correctly-initialized value.

So I was wondering if anyone could support the document's assertion with a reference, saying "yes, this is definitely safe, no memory-barrier needed, because of X, Y and Z...".  Or maybe the problem is theoretical and simply doesn't occur on any machine or implementation we care about?

Thanks,

Lambros

[Fred Akalin]

On Fri, Mar 23, 2012 at 12:11 PM, Lambros Lambrou <lambros...@chromium.org> wrote:

I had a look at this document, it's really good (one of the best I've read) but it left me with a couple of unanswered questions, and I'm hoping some experts reading this can set the record straight.

From the document: "Variables accessed by more than one thread and written by at least one thread should be protected consistently by a Lock."

Does this apply to booleans?  Is there a consensus on how booleans should be treated in Chrome code?  Do they need full locking?  Declaring as volatile OK?  Or don't bother?  My own understanding is that booleans do need some kind of locking, even though booleans are atomic, because there is a possibility of a change made in one thread not becoming visible in another thread.  Is this actually true on systems we care about?

Yes, booleans are no different, and do need locking (or explicit memory barriers with atomic ops).  Without memory barriers, the changed boolean may live in cache in one core and not be visible from others, so it's a practical concern.  BTW, 'volatile' has almost nothing to do with thread-safety.

 

My second question is about this assertion from the document: "An exception to this rule is that fields may be initialized in constructors without a mutex, since no other thread should be able to reference the memory at that time."

I seem to remember reading something a long time ago that contradicted this assertion (and claimed it to be a common myth).  Unfortunately, I can't remember where I read this, and haven't been able to find anything I'd trust as definitive.  The claim I read was (IIRC): if you allocate a new object, initialize one of the fields (in initialization list or ctor), then access the object from another thread without using a memory barrier, the other thread might see the "default" value of the field, and not the correctly-initialized value.

You may have a point here.  Considering a struct with a single bool that initializes it to 'true' and passing the address of the object to a different thread, the thread may not see the write.  I'll ask m3b to clarify; maybe he meant something else.

[Greg Thompson]

On Fri, Mar 23, 2012 at 3:11 PM, Lambros Lambrou <lambros...@chromium.org> wrote:

On Fri, Mar 23, 2012 at 10:20 AM, Fred Akalin <aka...@chromium.org> wrote:

Of course!  Everyone, feel free to expand and improve the document.

I had a look at this document, it's really good (one of the best I've read) but it left me with a couple of unanswered questions, and I'm hoping some experts reading this can set the record straight.

From the document: "Variables accessed by more than one thread and written by at least one thread should be protected consistently by a Lock."

Does this apply to booleans?  Is there a consensus on how booleans should be treated in Chrome code?  Do they need full locking?  Declaring as volatile OK?

Not okay; the volatile keyword is not a thread synchronization primitive.

 

 Or don't bother?  My own understanding is that booleans do need some kind of locking, even though booleans are atomic,

They aren't necessarily.  For simple cases where a bool may do the job, see base/synchronization/cancellation_flag.h.

 

My second question is about this assertion from the document: "An exception to this rule is that fields may be initialized in constructors without a mutex, since no other thread should be able to reference the memory at that time."

I seem to remember reading something a long time ago that contradicted this assertion (and claimed it to be a common myth).  Unfortunately, I can't remember where I read this, and haven't been able to find anything I'd trust as definitive.  The claim I read was (IIRC): if you allocate a new object, initialize one of the fields (in initialization list or ctor), then access the object from another thread without using a memory barrier, the other thread might see the "default" value of the field, and not the correctly-initialized value.

So I was wondering if anyone could support the document's assertion with a reference, saying "yes, this is definitely safe, no memory-barrier needed, because of X, Y and Z...".  Or maybe the problem is theoretical and simply doesn't occur on any machine or implementation we care about?

Thanks,

Lambros

[James Robinson]

"Passing the address of the object to a different thread" pretty much always implies a memory barrier of some sort.  In chromium, PostTask() has such a memory barrier so you can always be assured that any actions taken on the calling thread before the PostTask() will be observable to the thread running the task.  I think it is worth calling this out explicitly in the dev.chromium.org document since it's a key part of the most common synchronization method that we use in the Chromium project.

I think the point of this section is that unless you pass out a pointer to the object before initialization completes (say by having a member variable with a c'tor that PostTask()s a this pointer), initialization will be complete before any other threads get a chance to see the data.

- James

[Scott Hess]

On Fri, Mar 23, 2012 at 12:11 PM, Lambros Lambrou
<lambros...@chromium.org> wrote:

> On Fri, Mar 23, 2012 at 10:20 AM, Fred Akalin <aka...@chromium.org> wrote:
>> Of course!  Everyone, feel free to expand and improve the document.
>
> I had a look at this document, it's really good (one of the best I've read)
> but it left me with a couple of unanswered questions, and I'm hoping some
> experts reading this can set the record straight.
>
> From the document: "Variables accessed by more than one thread and written
> by at least one thread should be protected consistently by a Lock."
>
> Does this apply to booleans?  Is there a consensus on how booleans should be
> treated in Chrome code?  Do they need full locking?  Declaring as volatile
> OK?  Or don't bother?  My own understanding is that booleans do need some
> kind of locking, even though booleans are atomic, because there is a
> possibility of a change made in one thread not becoming visible in another
> thread.  Is this actually true on systems we care about?

In general, you should not reason your way around locking. Removing
the need to do locking by removing the need to access the value from
multiple threads, +1. But removing it by arguing that your access
patterns are so structured as to make it not necessary should only be
left to experts.

[To be clear, I am not suggesting that I am an expert, or that there
is some way to bless Chromium team members. Rather, history is
littered with bugs and security holes written by people reasoning
their way around locking primitives. It's just a hard problem.]

> My second question is about this assertion from the document: "An exception
> to this rule is that fields may be initialized in constructors without a
> mutex, since no other thread should be able to reference the memory at that
> time."
>
> I seem to remember reading something a long time ago that contradicted this
> assertion (and claimed it to be a common myth).  Unfortunately, I can't
> remember where I read this, and haven't been able to find anything I'd trust
> as definitive.  The claim I read was (IIRC): if you allocate a new object,
> initialize one of the fields (in initialization list or ctor), then access
> the object from another thread without using a memory barrier, the other
> thread might see the "default" value of the field, and not the
> correctly-initialized value.
>
> So I was wondering if anyone could support the document's assertion with a
> reference, saying "yes, this is definitely safe, no memory-barrier needed,
> because of X, Y and Z...".  Or maybe the problem is theoretical and simply
> doesn't occur on any machine or implementation we care about?

Being able to access the value without an intervening memory barrier
implies that someone was walking a shared data structure without
holding a lock. I think.

-scott

[William Chan (陈智昌)]

I'm very nervous about this type of conversation, because honestly, 99% of the time, you shouldn't be using Locks and WaitableEvents in the first place, but use message passing and asynchronous callbacks instead. Why does everyone think they're the exception?

[Antoine Labour]

On Fri, Mar 23, 2012 at 12:11 PM, Lambros Lambrou <lambros...@chromium.org> wrote:

On Fri, Mar 23, 2012 at 10:20 AM, Fred Akalin <aka...@chromium.org> wrote:

Of course!  Everyone, feel free to expand and improve the document.

I had a look at this document, it's really good (one of the best I've read) but it left me with a couple of unanswered questions, and I'm hoping some experts reading this can set the record straight.

From the document: "Variables accessed by more than one thread and written by at least one thread should be protected consistently by a Lock."

Does this apply to booleans?  Is there a consensus on how booleans should be treated in Chrome code?  Do they need full locking?

Yes

 

 Declaring as volatile OK?

volatile doesn't do what you think it does. It only tells the compiler how to generate the code (force a memory access, enforce compiler-level ordering rules), but it's orthogonal to cpu-level reordering.

 

 Or don't bother?  My own understanding is that booleans do need some kind of locking, even though booleans are atomic, because there is a possibility of a change made in one thread not becoming visible in another thread.  Is this actually true on systems we care about?

yes

My second question is about this assertion from the document: "An exception to this rule is that fields may be initialized in constructors without a mutex, since no other thread should be able to reference the memory at that time."

I seem to remember reading something a long time ago that contradicted this assertion (and claimed it to be a common myth).  Unfortunately, I can't remember where I read this, and haven't been able to find anything I'd trust as definitive.  The claim I read was (IIRC): if you allocate a new object, initialize one of the fields (in initialization list or ctor), then access the object from another thread without using a memory barrier, the other thread might see the "default" value of the field, and not the correctly-initialized value.

So I was wondering if anyone could support the document's assertion with a reference, saying "yes, this is definitely safe, no memory-barrier needed, because of X, Y and Z...".  Or maybe the problem is theoretical and simply doesn't occur on any machine or implementation we care about?

You need a memory barrier, but in chrome code, you have an implicit one when, say, you post a task to the other thread. Constructing a case where you'd pass the object to another thread without a memory barrier (and otherwise correct) is pretty hard. You need to pass the object through some shared state, that you'd need to lock to modify. One exception is a global object with static construction. But those are forbidden.

Antoine

 

Thanks,

Lambros

[John Bates]

On Fri, Mar 23, 2012 at 12:34 PM, Antoine Labour <pi...@google.com> wrote:

On Fri, Mar 23, 2012 at 12:11 PM, Lambros Lambrou <lambros...@chromium.org> wrote:

On Fri, Mar 23, 2012 at 10:20 AM, Fred Akalin <aka...@chromium.org> wrote:

Of course!  Everyone, feel free to expand and improve the document.

I had a look at this document, it's really good (one of the best I've read) but it left me with a couple of unanswered questions, and I'm hoping some experts reading this can set the record straight.

From the document: "Variables accessed by more than one thread and written by at least one thread should be protected consistently by a Lock."

Does this apply to booleans?  Is there a consensus on how booleans should be treated in Chrome code?  Do they need full locking?

Yes

 

 Declaring as volatile OK?

volatile doesn't do what you think it does. It only tells the compiler how to generate the code (force a memory access, enforce compiler-level ordering rules), but it's orthogonal to cpu-level reordering.

 

 Or don't bother?  My own understanding is that booleans do need some kind of locking, even though booleans are atomic, because there is a possibility of a change made in one thread not becoming visible in another thread.  Is this actually true on systems we care about?

yes

On ARM, for example, where implementations may not be fully cache coherent. That means you need either locks or base::subtle atomic ops to ensure that what is seen at address X by one thread is also seen by other threads.

Note that this isn't about whether some of the bits of the bool get updated while others do not (there's another debate here about how big of a type can be expected to be written to memory / cached as one atomic chunk). It's instead about whether all the cached copies of the value at address X have been updated with its new value.

X86 guarantees cache coherency as part of its spec (meaning that as long as you've convinced the compiler to execute store / load instructions in the right order, all threads have a consistent view of memory -- but the specifics of what you can get away with are still very subtle). ARM sadly left the decision of cache coherency to vendors.

 

My second question is about this assertion from the document: "An exception to this rule is that fields may be initialized in constructors without a mutex, since no other thread should be able to reference the memory at that time."

I seem to remember reading something a long time ago that contradicted this assertion (and claimed it to be a common myth).  Unfortunately, I can't remember where I read this, and haven't been able to find anything I'd trust as definitive.  The claim I read was (IIRC): if you allocate a new object, initialize one of the fields (in initialization list or ctor), then access the object from another thread without using a memory barrier, the other thread might see the "default" value of the field, and not the correctly-initialized value.

So I was wondering if anyone could support the document's assertion with a reference, saying "yes, this is definitely safe, no memory-barrier needed, because of X, Y and Z...".  Or maybe the problem is theoretical and simply doesn't occur on any machine or implementation we care about?

You need a memory barrier, but in chrome code, you have an implicit one when, say, you post a task to the other thread. Constructing a case where you'd pass the object to another thread without a memory barrier (and otherwise correct) is pretty hard. You need to pass the object through some shared state, that you'd need to lock to modify. One exception is a global object with static construction. But those are forbidden.

Antoine

 

Thanks,

Lambros

[John Abd-El-Malek]

+1 I've been trying to avoid commenting on this thread, but I share the concern that this document will encourage people to add locks, which is not what we want to do.