Proposal: base/thread_annotations.h from Abseil

[Victor Costan]

Although most of Chromium code relies on message-passing rather than mutexes, we still use mutexes in the codebase.

Abseil has some very useful (imho) macros for annotating Mutex usage:

https://github.com/abseil/abseil-cpp/blob/master/absl/base/thread_annotations.h

Coding is hard, and it's nice to let compilers help us. What do you think about adding this header (or perhaps a subset) to Chromium, and allowing it in Blink? Interested folks (myself included) can follow up by annotating our mutex classes.

Thank you in advance for your thoughts!

    Victor

[Victor Costan]

I forgot to mention that the macros are also directly supported by and documented in Clang: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

I included the cxx list because I thought of the macros as somewhat of a language feature, because of the Clang support mentioned above.

Sorry, and thank you for putting up with the extra post!

    Victor

[Kentaro Hara]

Given the philosophy that Blink should share as much code with Chromium as possible, my opinion is: If //base/ decides to adopt this, I want to adopt it in Blink as well :)

Have you reached out to //base/OWNERS?

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAP_mGKoQJEempKK5Mc2__c9M%3DETfS0xH4wBP2z8buM_yt_f%3Dhg%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

[Victor Costan]

+thestig@: Please take a look at the proposal in this thread, quoted below.

On Wed, Jan 24, 2018 at 4:23 PM, Kentaro Hara <har...@chromium.org> wrote:

Given the philosophy that Blink should share as much code with Chromium as possible, my opinion is: If //base/ decides to adopt this, I want to adopt it in Blink as well :)

Have you reached out to //base/OWNERS?

Thank you for the great suggestion! I just added thestig@, and I think that the other //base/OWNERS are members of cxx@.

[Nico Weber]

I vaguely remember that we had a thread on thread annotations somewhere in the past (maybe form jamesr@) -- if you haven't, maybe try looking for it, maybe it has interesting points (if it in fact exists).

Generally seems fine to me (assuming the thread annotation processing in the compiler doesn't measurably slow down builds -- unlikely but we should measure), but given that we generally prefer message passing and other higher-level primitives over mutexes and locks, I'd imagine that this won't be useful all that often.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAP_mGKprHN8Vyk6GZ9UvpS_MPc_HrNGakeFQV%3DJ-WrygT0kA7A%40mail.gmail.com.

[Victor Costan]

On Wed, Jan 24, 2018 at 5:23 PM, Nico Weber <tha...@chromium.org> wrote:

I vaguely remember that we had a thread on thread annotations somewhere in the past (maybe form jamesr@) -- if you haven't, maybe try looking for it, maybe it has interesting points (if it in fact exists).

Thank you for the pointer! I think the thread is -- https://groups.google.com/a/chromium.org/d/topic/chromium-dev/4F0PYt2YrW0/discussion

It seems like you were in favor back then, with the same reservation regarding build times. It also seems like the conversation just died down.

Generally seems fine to me (assuming the thread annotation processing in the compiler doesn't measurably slow down builds -- unlikely but we should measure), but given that we generally prefer message passing and other higher-level primitives over mutexes and locks, I'd imagine that this won't be useful all that often.

I am proposing this because I stumbled over some code in Blink that does use mutexes. The annotations are already used by other subprojects, like Mojo (mentioned before), WebRTC, and tcmalloc.

//net/quick also appears to have blank definitions of the annotations: https://cs.chromium.org/chromium/src/net/quic/platform/impl/quic_mutex_impl.h

Given existing use, I think it'd be useful to have a "base/thread_annotations.h" that matches the macros offered by Abseil.

In order to avoid this conversation dying down as well, I propose prototyping a CL, if there's no objection in the next couple of days.

[Daniel Cheng]

Having something in //base for this makes sense to me. While I don't expect it to be widely used, it can't hurt, and it's certainly nicer than purely advisory comments.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAP_mGKpEy2JjXikomhcMQao-NyjWC-4ib%2B6vaM5%2Bcexc0np58Q%40mail.gmail.com.

[Nico Weber]

On Wed, Jan 24, 2018 at 8:49 PM, Victor Costan <pwn...@chromium.org> wrote:

On Wed, Jan 24, 2018 at 5:23 PM, Nico Weber <tha...@chromium.org> wrote:

I vaguely remember that we had a thread on thread annotations somewhere in the past (maybe form jamesr@) -- if you haven't, maybe try looking for it, maybe it has interesting points (if it in fact exists).

Thank you for the pointer! I think the thread is -- https://groups.google.com/a/chromium.org/d/topic/chromium-dev/4F0PYt2YrW0/discussion

It seems like you were in favor back then, with the same reservation regarding build times. It also seems like the conversation just died down.

Generally seems fine to me (assuming the thread annotation processing in the compiler doesn't measurably slow down builds -- unlikely but we should measure), but given that we generally prefer message passing and other higher-level primitives over mutexes and locks, I'd imagine that this won't be useful all that often.

I am proposing this because I stumbled over some code in Blink that does use mutexes. The annotations are already used by other subprojects, like Mojo (mentioned before), WebRTC, and tcmalloc.

//net/quick also appears to have blank definitions of the annotations: https://cs.chromium.org/chromium/src/net/quic/platform/impl/quic_mutex_impl.h

Given existing use, I think it'd be useful to have a "base/thread_annotations.h" that matches the macros offered by Abseil.

In order to avoid this conversation dying down as well, I propose prototyping a CL, if there's no objection in the next couple of days.

Sounds great, thanks!

[Gabriel Charette]

FWIW, we needed this in //base/task_scheduler and added //base/task_scheduler/scheduler_lock.h (which is just a base::Lock in release builds) to avoid imposing this on //base at the time. I'm in favor of adding mutex annotations to //base. When we do we should cleanup hand-crafted constructs such as SchedulerLock.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiH_a2rqVPpZgMDGKkzLcoL%2BjqWV2Lgakubxf1pNCRmyZw%40mail.gmail.com.

[Gabriel Charette]

(re-send after joining blink-dev@ -- apparently messages are blocked otherwise)

[Primiano Tucci]

From the viewpoint of a potential user of this feature, that IMHO sounds great if it's feasible. We still have several places where, for various reasons, we use locks and it's hard to avoid it. I had myself a bunch of bugs of the form "this variable should have been accessed only under this lock, but I screwed that in a following refactor" where these annotations would have saved me quite some debugging time. 

Thanks for looking into this!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LLnZzCcJzma46Pg9X8EOhTH%2BRE-B5f%3Df0PH9cGsxA%2Bf0w%40mail.gmail.com.

[Alex Clarke]

it's very easy to get threadding wrong. To try and combat that in the blink scheduler we've been using somewhat ugly accessor functions  main_thread_only().xyz and any_thread().abc (which assert that the variable is accessed from the right thread, or that a lock is held).  Perhaps this could replace the any_thread() accessor.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CA%2ByH71dWzZFhOVm9Lp-MwNXc7e2%3DR7uGKVGpwbx%3DKvwBokvZOQ%40mail.gmail.com.

[Gabriel Charette]

Another datapoint: a while back I added ThreadSafeStateManager in SafeBrowsingDatabase to enforce Lock acquisition when accessing some state (had been source of bugs before then). This is currently a type in official builds as well. Could be superseded by annotations :).

[g...@chromium.org]

On Thursday, January 25, 2018 at 12:50:30 PM UTC+1, Gabriel Charette wrote:

Another datapoint: a while back I added ThreadSafeStateManager in SafeBrowsingDatabase to enforce Lock acquisition when accessing some state (had been source of bugs before then). This is currently a type in official builds as well. Could be superseded by annotations :).

On Thu, Jan 25, 2018 at 12:43 PM Alex Clarke <alexc...@google.com> wrote:

it's very easy to get threadding wrong. To try and combat that in the blink scheduler we've been using somewhat ugly accessor functions  main_thread_only().xyz and any_thread().abc (which assert that the variable is accessed from the right thread, or that a lock is held).  Perhaps this could replace the any_thread() accessor.

On 25 January 2018 at 11:31, Primiano Tucci <prim...@chromium.org> wrote:

From the viewpoint of a potential user of this feature, that IMHO sounds great if it's feasible. We still have several places where, for various reasons, we use locks and it's hard to avoid it. I had myself a bunch of bugs of the form "this variable should have been accessed only under this lock, but I screwed that in a following refactor" where these annotations would have saved me quite some debugging time. 

Thanks for looking into this!

On Thu, Jan 25, 2018 at 9:24 AM Gabriel Charette <g...@chromium.org> wrote:

(re-send after joining blink-dev@ -- apparently messages are blocked otherwise)

On Thu, Jan 25, 2018 at 10:04 AM Gabriel Charette <g...@chromium.org> wrote:

FWIW, we needed this in //base/task_scheduler and added //base/task_scheduler/scheduler_lock.h (which is just a base::Lock in release builds) to avoid imposing this on //base at the time. I'm in favor of adding mutex annotations to //base. When we do we should cleanup hand-crafted constructs such as SchedulerLock.

On Thu, Jan 25, 2018 at 3:25 AM Nico Weber <tha...@chromium.org> wrote:

On Wed, Jan 24, 2018 at 8:49 PM, Victor Costan <pwn...@chromium.org> wrote:

On Wed, Jan 24, 2018 at 5:23 PM, Nico Weber <tha...@chromium.org> wrote:

I vaguely remember that we had a thread on thread annotations somewhere in the past (maybe form jamesr@) -- if you haven't, maybe try looking for it, maybe it has interesting points (if it in fact exists).

Thank you for the pointer! I think the thread is -- https://groups.google.com/a/chromium.org/d/topic/chromium-dev/4F0PYt2YrW0/discussion

It seems like you were in favor back then, with the same reservation regarding build times. It also seems like the conversation just died down.

Generally seems fine to me (assuming the thread annotation processing in the compiler doesn't measurably slow down builds -- unlikely but we should measure), but given that we generally prefer message passing and other higher-level primitives over mutexes and locks, I'd imagine that this won't be useful all that often.

I am proposing this because I stumbled over some code in Blink that does use mutexes. The annotations are already used by other subprojects, like Mojo (mentioned before), WebRTC, and tcmalloc.

//net/quick also appears to have blank definitions of the annotations: https://cs.chromium.org/chromium/src/net/quic/platform/impl/quic_mutex_impl.h

Given existing use, I think it'd be useful to have a "base/thread_annotations.h" that matches the macros offered by Abseil.

In order to avoid this conversation dying down as well, I propose prototyping a CL, if there's no objection in the next couple of days.

Any updates here? I'm eager to get this in our codebase :)!

Another useful datapoint, this would avoid manually tagging methods as FooLockRequired() and variants (examples in //base).

[Nico Weber]

On Tue, Feb 13, 2018 at 4:45 AM, <g...@chromium.org> wrote:

On Thursday, January 25, 2018 at 12:50:30 PM UTC+1, Gabriel Charette wrote:

Another datapoint: a while back I added ThreadSafeStateManager in SafeBrowsingDatabase to enforce Lock acquisition when accessing some state (had been source of bugs before then). This is currently a type in official builds as well. Could be superseded by annotations :).

On Thu, Jan 25, 2018 at 12:43 PM Alex Clarke <alexc...@google.com> wrote:

it's very easy to get threadding wrong. To try and combat that in the blink scheduler we've been using somewhat ugly accessor functions  main_thread_only().xyz and any_thread().abc (which assert that the variable is accessed from the right thread, or that a lock is held).  Perhaps this could replace the any_thread() accessor.

On 25 January 2018 at 11:31, Primiano Tucci <prim...@chromium.org> wrote:

From the viewpoint of a potential user of this feature, that IMHO sounds great if it's feasible. We still have several places where, for various reasons, we use locks and it's hard to avoid it. I had myself a bunch of bugs of the form "this variable should have been accessed only under this lock, but I screwed that in a following refactor" where these annotations would have saved me quite some debugging time. 

Thanks for looking into this!

On Thu, Jan 25, 2018 at 9:24 AM Gabriel Charette <g...@chromium.org> wrote:

(re-send after joining blink-dev@ -- apparently messages are blocked otherwise)

On Thu, Jan 25, 2018 at 10:04 AM Gabriel Charette <g...@chromium.org> wrote:

FWIW, we needed this in //base/task_scheduler and added //base/task_scheduler/scheduler_lock.h (which is just a base::Lock in release builds) to avoid imposing this on //base at the time. I'm in favor of adding mutex annotations to //base. When we do we should cleanup hand-crafted constructs such as SchedulerLock.

On Thu, Jan 25, 2018 at 3:25 AM Nico Weber <tha...@chromium.org> wrote:

On Wed, Jan 24, 2018 at 8:49 PM, Victor Costan <pwn...@chromium.org> wrote:

On Wed, Jan 24, 2018 at 5:23 PM, Nico Weber <tha...@chromium.org> wrote:

I vaguely remember that we had a thread on thread annotations somewhere in the past (maybe form jamesr@) -- if you haven't, maybe try looking for it, maybe it has interesting points (if it in fact exists).

Thank you for the pointer! I think the thread is -- https://groups.google.com/a/chromium.org/d/topic/chromium-dev/4F0PYt2YrW0/discussion

It seems like you were in favor back then, with the same reservation regarding build times. It also seems like the conversation just died down.

Generally seems fine to me (assuming the thread annotation processing in the compiler doesn't measurably slow down builds -- unlikely but we should measure), but given that we generally prefer message passing and other higher-level primitives over mutexes and locks, I'd imagine that this won't be useful all that often.

I am proposing this because I stumbled over some code in Blink that does use mutexes. The annotations are already used by other subprojects, like Mojo (mentioned before), WebRTC, and tcmalloc.

//net/quick also appears to have blank definitions of the annotations: https://cs.chromium.org/chromium/src/net/quic/platform/impl/quic_mutex_impl.h

Given existing use, I think it'd be useful to have a "base/thread_annotations.h" that matches the macros offered by Abseil.

In order to avoid this conversation dying down as well, I propose prototyping a CL, if there's no objection in the next couple of days.

Any updates here? I'm eager to get this in our codebase :)!

Good news, you can! They landed here: https://chromium-review.googlesource.com/c/chromium/src/+/885506

[Gabriel Charette]

Oh! Totally missed it, thanks! Added comments @ https://chromium-review.googlesource.com/c/chromium/src/+/885506#message-f9080332e4f52638e2d128583dc57d17d16367dc

[Victor Costan]

On Tue, Feb 13, 2018 at 6:08 AM, Gabriel Charette <g...@chromium.org> wrote:

On Tue, Feb 13, 2018 at 2:44 PM Nico Weber <tha...@chromium.org> wrote:

On Tue, Feb 13, 2018 at 4:45 AM, <g...@chromium.org> wrote:

Any updates here? I'm eager to get this in our codebase :)!

Good news, you can! They landed here: https://chromium-review.googlesource.com/c/chromium/src/+/885506

Oh! Totally missed it, thanks! Added comments @ https://chromium-review.googlesource.com/c/chromium/src/+/885506#message-f9080332e4f52638e2d128583dc57d17d16367dc

I'm sorry, I forgot to hit "send" on a reply to this thread after the CL landed.

base/thread_annotations.h is available, and hooked into Blink. It is not yet hooked into base::Lock, because of this LLVM bug, which is needed to analyze base::AutoUnlock. I hope to have time to figure out the LLVM codebase and the bug sometime in the next few weeks. If anyone else has the time beforehand, please don't be shy about stealing the bug from me.

    Victor

[Gabriel Charette]

Great, thanks! Shouldn't GUARDED_BY(lock_) fail though? Right now it compiles without warnings but doesn't do anything.

 

    Victor