757969 - Add support for __thread and/or __declspec(thread) to ThreadLocal

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For performance. See bug 703317.

Comment 1

[Mike Hommey [:glandium]]

Unfortunately, we can't use __declspec(thread) because all our libraries are dynamically loaded:
http://msdn.microsoft.com/en-us/library/9w1sdazb%28v=vs.80%29.aspx#1

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One day...

Comment 3

[Tom Tromey :tromey]

I took a quick stab at this yesterday.  I can attach my WIP patch if you want.
I only tried it on Linux; it checks HAVE_THREAD_TLS_KEYWORD and falls
back to the current behavior when this is not defined.  I didn't run the test suite, just
started the browser to see it not crash.

There are a couple of gotchas though.

The most important one is bug 1064672.  There's code in GeckoTaskTracer that
wants to allocate a ThreadLocal on the heap:

  sTraceInfoTLS = new ThreadLocal<TraceInfo*>();

I don't know enough about this code to suggest what to do.


The other oddities are more minor and stem from the fact that __thread
is a storage class specifier.  Currently ThreadLocal conceptually mixes
the per-thread storage with some global state -- the initialized bit.

My approach was to make the ThreadLocal just a small wrapper around the
contained value and mark all definitions and declarations with __thread.
I did this with a new macro that must be used rather than explicitly
writing "ThreadLocal".

Then, in the __thread mode, I have the init() and initialized() do nothing
and simply return true.

I also had to make the init() method idempotent to deal with code that
checked the initialized bit before calling init(), e.g. NS_SetMainThread.


Another possible approach here would be to make a __thread member of ThreadLocal.
However, this would require a unique instantiation of ThreadLocal for each
definition.  I couldn't think of a pretty way to accomplish this.

Comment 4

[Jeff Walden [:Waldo]]

Heap-allocating ThreadLocal, or allocating it any way other than with static duration, seems very very likely to me to be an unsupported use case.  We explicitly say, as I recall, that you can only allocate it with static duration.  So I wouldn't worry about that.  (There's a good chance the approaches in bug 1064672 will be r-'d, but I'm hesitant to do that right now, having only seen the last bug modification just now, with me in a somewhat sleep-deprived state due to a cross-country redeye last night.  Maybe Monday.)

Comment 5

[Tom Tromey :tromey]

Attachment: one approach to using __thread

Here's my patch.  See earlier comments for the caveats.  I'm sure it
won't build on some platforms and it just pretends away the
heap-allocated ThreadLocal issue.  Perhaps still useful regardless.

Comment 6

[Tom Tromey :tromey]

Attachment: use __thread in ThreadLocal

Rebased and updated.

This may still need some changes to work properly on all platforms;
will try soon.

Comment 7

[Tom Tromey :tromey]

Attachment: use __thread in ThreadLocal

This one doesn't cause "mach gtest" to die on my machine.

Comment 8

[Tom Tromey :tromey]

Might as well admit it.

Comment 9

[Tom Tromey :tromey]

Attachment: use __thread in ThreadLocal

Fix up uses in UncensoredAllocator.cpp as well.

Comment 10

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e245337fb963

Comment 11

[Tom Tromey :tromey]

Nathan, at Orlando you mentioned that this bug might cause problems for some OS X versions.
The try run looks pretty clean - I'm wondering what I might be missing.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Tom Tromey :tromey from comment #11)
> Nathan, at Orlando you mentioned that this bug might cause problems for some
> OS X versions.
> The try run looks pretty clean - I'm wondering what I might be missing.

You're missing a conversational partner who knows what he's talking about. ;)

Where does THREAD_HAVE_TLS_KEYWORD come from?  Is that autoconf magic?

Comment 13

[Tom Tromey :tromey]

(In reply to Nathan Froyd [:froydnj] from comment #12)

> Where does THREAD_HAVE_TLS_KEYWORD come from?  Is that autoconf magic?

Yep:

https://dxr.mozilla.org/mozilla-central/source/configure.in#3270

Comment 14

[Mike Hommey [:glandium]]

(In reply to Tom Tromey :tromey from comment #11)
> Nathan, at Orlando you mentioned that this bug might cause problems for some
> OS X versions.
> The try run looks pretty clean - I'm wondering what I might be missing.

It's not causing problems because you're using THREAD_HAVE_TLS_KEYWORD, but here is what the builds have to say about it:

checking for __thread keyword for TLS variables... no

Comment 15

[Tom Tromey :tromey]

(In reply to Mike Hommey (VAC: 31 Dec - 11 Jan) [:glandium] from comment #14)
> (In reply to Tom Tromey :tromey from comment #11)
> > Nathan, at Orlando you mentioned that this bug might cause problems for some
> > OS X versions.
> > The try run looks pretty clean - I'm wondering what I might be missing.
> 
> It's not causing problems because you're using THREAD_HAVE_TLS_KEYWORD, but
> here is what the builds have to say about it:
> 
> checking for __thread keyword for TLS variables... no

Thanks for noticing that.
This means the patch is working as intended then.

Comment 16

[Tom Tromey :tromey]

Comment on attachment 8704692 [details] [diff] [review]
use __thread in ThreadLocal

This patch does what I need for bug 1232712 and still leaves it possible to
port thread-locals to new platforms.

We discussed problems with some versions of OS X in Orlando, but configure
already filters those out.  This is ok for 1232712 as that code currently
only works on Linux anyway.

I made ThreadLocal_::initialized private now.  Formerly some code was checking
this; but this isn't really possible with true TLS, and such code now has
to keep its own flag.

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8704692 [details] [diff] [review]
use __thread in ThreadLocal

Review of attachment 8704692 [details] [diff] [review]:
-----------------------------------------------------------------

All this looks pretty reasonable.  r=me with the changes below.

::: mfbt/ThreadLocal.h
@@ +52,5 @@
>   * Usage:
>   *
> + * Do not directly instantiate this class.  Instead, use the
> + * MOZ_THREAD_LOCAL macro to declare or define instances.  The macro
> + * takes a type name as its argument.

I think it'd be preferred to define ThreadLocal in mozilla::detail to further encourage people to not instantiate it directly.

@@ +73,5 @@
>   * //
> + * // Note that init() should be invoked before the first use of set()
> + * // or get().  It is ok to call it multiple times.  It is
> + * // recommended to call it in a way that avoids possible races
> + * // with other threads.

Is that a "recommended" with a subtext of "things will go badly for you if you don't"?

If so, we should make the documentation stronger on this point.

@@ +86,5 @@
>   * // Get the TLS value
>   * int value = tlsKey.get();
>   */
>  template<typename T>
> +class ThreadLocal_

Please remove the extra underscore, as it will be unnecessary with the detail:: change below.

We could do:

enum ThreadLocalModel {
  Windows,
  Posix,
  ThreadKeyword,
};

template<typename T, enum ThreadLocalModel>
class ThreadLocal;

with specializations taking the place of some of the ifdefs below.  But this is already heavily-ifdef'd code, so I don't think this is terribly compelling.  It could be an interesting cleanup for future work.

::: xpcom/base/nsCycleCollector.cpp
@@ -3999,5 @@
>  bool
>  nsCycleCollector_init()
>  {
>    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
> -  MOZ_ASSERT(!sCollectorData.initialized(), "Called twice!?");

No sCollectorDataInitialized flag to replace this use and the one below?  I guess it might be a bit of a pain to make it DEBUG-only.

Comment 18

[Tom Tromey :tromey]

(In reply to Nathan Froyd [:froydnj] from comment #17)


> > + * // Note that init() should be invoked before the first use of set()
> > + * // or get().  It is ok to call it multiple times.  It is
> > + * // recommended to call it in a way that avoids possible races
> > + * // with other threads.
> 
> Is that a "recommended" with a subtext of "things will go badly for you if
> you don't"?
> 
> If so, we should make the documentation stronger on this point.

I've changed it to

 * // Note that init() should be invoked before the first use of set()
 * // or get().  It is ok to call it multiple times.  This must be
 * // called in a way that avoids possible races with other threads.

This isn't actually a new requirement; I just thought it was worth mentioning.

> template<typename T, enum ThreadLocalModel>
> class ThreadLocal;

I took a brief look at this.  It replaces the #if stuff nicely, but on the other
hand means a lot of code duplication (the entire body of ThreadLocal plus
all the methods).  I can do it if you want it though.

> ::: xpcom/base/nsCycleCollector.cpp
> @@ -3999,5 @@
> >  bool
> >  nsCycleCollector_init()
> >  {
> >    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
> > -  MOZ_ASSERT(!sCollectorData.initialized(), "Called twice!?");
> 
> No sCollectorDataInitialized flag to replace this use and the one below?  I
> guess it might be a bit of a pain to make it DEBUG-only.

I only added them where they seemed genuinely necessary.
I don't mind adding one here, maybe a static local DebugOnly.

Comment 19

[Tom Tromey :tromey]

Attachment: use __thread in ThreadLocal

Updated per review comments.

Rebased.  ThreadLocal grew a constructor; I suppose because we don't
have an attribute (and static analysis pass) meaning "must be static
or global".

A __thread variable can't have a non-trivial constructor.  However,
since __thread enforces the "static or global" rule, it seems to me
that no constructor is needed.  So, this update simply #ifs it out in
the __thread case.

Comment 20

[Tom Tromey :tromey]

NI'ing you for a couple things:

* See comment #19 for changes in the patch, in case you want to weigh in
* Do you want new bugs for ports to OSX and Windows?  Or for the
  template specialization idea from comment #17?

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to Tom Tromey :tromey from comment #20)
> NI'ing you for a couple things:
> 
> * See comment #19 for changes in the patch, in case you want to weigh in

I think it might be worth a comment as to why we're #ifdef'ing away the constructor.

> * Do you want new bugs for ports to OSX and Windows?  Or for the
>   template specialization idea from comment #17?

A new bug for the template specialization idea would be good.  I'm unsure what "ports to OSX and Windows" means.  Do you mean using the __thread-equivalents on OSX and Windows?  If there is such a facility on OSX, then we should file a bug about it.  Given comment 1, we can't do this on Windows. :(

Comment 22

[Tom Tromey :tromey]

Attachment: use __thread in ThreadLocal

Add a comment.

Comment 23

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cccc6ff7b783

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/673d16803c0c

Comment 25

[Tom Tromey :tromey]

A more recent try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3e20abf146c

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/673d16803c0c