Consider dropping mozilla::ThreadLocal and using native thread_local instead

NEW
Unassigned

Status

()

2 years ago
a year ago

People

(Reporter: xidorn, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox53 affected)

Details

(Reporter)

Description

2 years ago
As noted in Using C++ in Mozilla code [1], thread_local has been supported in all compilers we use. It was not allowed to be used because of breaking Windows XP and OS X 10.6. However, now we have dropped support to both Windows XP and OS X 10.6, we should be able to use the native thread_local now, and drop mozilla::ThreadLocal.

[1] https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code
(Reporter)

Updated

2 years ago
Depends on: 1130266
IIRC, Android doesn't support thread local storage, which may or may not impact the availability of thread_local. So you should check what the story is on Android, too.
(Reporter)

Comment 2

2 years ago
I searched "ndk thread_local" and didn't find many complaint about its not being supported. There is an issue reports that Clang with LTO fails to compile program using thread_local [1], and in that issue, the reporter said "Everything works without -flto." which indicates that Android is supposed to support thread_local.

IIRC we still uses GCC for Android build, so Clang LTO may not be an issue for us for now.

[1] https://github.com/android-ndk/ndk/issues/156

Comment 3

2 years ago
FWIW, I don't think there's any reason we wouldn't switch to thread_local once we 1) have compiler support everywhere, and 2) that support is "optimal" (if memory serves, this means memory for the thread-local is allocated at thread allocation time).

I think we'd want to (at a minimum) discourage use of thread_local with classes with non-trivial destructors, requiring people to manually manage thread_local pointers (or some similar scheme) for more complicated objects.  But given thread_local only supports pointer-like things now (and we don't really support things with destructors), that shouldn't be any barrier to switching.

Comment 4

2 years ago
Moreover, given that mozilla::ThreadLocal appears to be using pthread_getspecific/TlsGetValue, I think we'd get definite *speedup* switching to thread_local which, last I looked, had much better codegen.
(In reply to Luke Wagner [:luke] from comment #4)
> Moreover, given that mozilla::ThreadLocal appears to be using
> pthread_getspecific/TlsGetValue, I think we'd get definite *speedup*
> switching to thread_local which, last I looked, had much better codegen.

ThreadLocal uses __thread on POSIX-y systems if the compiler supports it nowadays.  I think now that we've dropped XP/Vista support, we could use __declspec(thread) on Windows and get the same benefits?

Comment 6

2 years ago
Oh, nice, n/m then :)
There was talk around TLS stuff at the quantum DOM meetings in Hawaii.  Bill might have opinions here.
I don't think this change should have much impact on Quantum DOM. If we need custom TLS functionality in certain places, we can just switch classes as needed. Whether we switch from ThreadLocal or thread_local isn't a big deal.
Blocks: 1130266
No longer depends on: 1130266

Comment 9

a year ago
See bug 1348419 for some efforts.

In particular, there we found that GCC 4.8 release notes say that __thread is still faster than thread_local:
https://gcc.gnu.org/gcc-4.8/changes.html#cxx

says:

> Unfortunately, this support requires a run-time penalty for references to non-function-local thread_local variables defined in a different translation unit even if they don't need dynamic initialization, so users may want to continue to use __thread for TLS variables with static initialization semantics. 

Removing the mozilla::ThreadLocal wrapper is still possible.
Duplicate of this bug: 1083507
FWIW in bug 1348419 it turned out that dealing with OSX and Android was too much of a pain,
in the end these still need ThreadLocal.  For Androi it is easy to test: just do a try run
using __thread or thread_local -- the bug was linker errors.  For OSX the problem was that
thread_local was not supported by a version of xcode a bit older than whatever is in use
on try.
You need to log in before you can comment on or make changes to this bug.