Use TLS storage for thread ids in wtf/ThreadingPThreads

Use TLS storage for thread ids in wtf/ThreadingPThreads

This reduces currentThread() overhead by > ~95-99% on Linux (on my Z620).

BUG=442246
Review-Url: https://codereview.chromium.org/2627153004
Cr-Commit-Position: refs/heads/master@{#443701}
Committed: https://chromium.googlesource.com/chromium/src/+/6deb801a3e4cd94c3a472b4e67520653afbbd380

[Charlie Harrison, 2017-01-12 06:04:11]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 06:04:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 06:18:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 06:19:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Charlie Harrison, 2017-01-12 16:23:15]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 16:23:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-01-12 17:56:55]

Description was changed from

==========
[WIP] Use TLS storage for thread ids in wtf/ThreadingPThreads

This speeds up currentThread() by a lot [1]

[1] to be measured

BUG=
==========

to

==========
Use TLS storage for thread ids in wtf/ThreadingPThreads

This speeds up currentThread() by a ~95% on Linux (on my Z620).

BUG=TODO
==========

[Charlie Harrison, 2017-01-12 18:21:08]

Description was changed from

==========
Use TLS storage for thread ids in wtf/ThreadingPThreads

This speeds up currentThread() by a ~95% on Linux (on my Z620).

BUG=TODO
==========

to

==========
Use TLS storage for thread ids in wtf/ThreadingPThreads

This reduces currentThread() overhead by > ~95% on Linux (on my Z620).

BUG=TODO
==========

[Charlie Harrison, 2017-01-12 18:25:09]

csharrison@chromium.org changed reviewers:
+ haraken@chromium.org

[Charlie Harrison, 2017-01-12 18:25:10]

Haraken,
I think this is unrelated to crbug.com/621786 which (I think) is about doing the
stack based heuristic for isMainThread. That is worthwhile, but adding TLS is
some separate low hanging fruit here.

This CL is WIP and I have a few questions:
1. ThreadState has specific mechanisms to initialize the TLS upon thread
initialization. I avoided that because I think it requires reaching in the
embedder's logic to add an initialize call, which would increase the complexity
of this patch, and add some odd layering. The downside is that currentThread()
has an extra branch for if the value is uninitialized.

2. We are leaking the ThreadSpecific (note the lsan failures). Is this ok to
annotate intentionally? I think fixing it would require something like (1),
where we get notified on thread termination.

Generally, ThreadState is in an easier position because ThreadStates only attach
to GC enabled threads, which we have more control over in Blink.

I think this CL is all that is left blocking enabling the thread checker on
StringImpl, but I haven't set this up as a dependent PS yet to check. In
profiling previous failures I found huge perf improvements though.

[commit-bot: I haz the power, 2017-01-12 18:26:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 18:26:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[esprehn, 2017-01-12 18:41:29]

Can we put this in wtfThreadData() instead? wtfThreadData().threadId() instead?

[Charlie Harrison, 2017-01-12 19:02:27]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 19:02:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-01-12 19:03:38]

Great idea, done! I didn't even realize that class existed :P How does it avoid
LSAN errors?

[Charlie Harrison, 2017-01-12 19:20:33]

csharrison@chromium.org changed reviewers:
+ esprehn@chromium.org

[Charlie Harrison, 2017-01-12 19:20:34]

re-adding esprehn to reviewer who somehow lost the cc. (I've done as you
suggested in case you missed the previous comment).

[commit-bot: I haz the power, 2017-01-12 19:35:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 19:35:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[esprehn, 2017-01-12 19:38:50]

lgtm, failed to compile though, did you forget to include something?

FAILED: obj/third_party/WebKit/Source/wtf/wtf/ThreadingPthreads.o 
export DEVELOPER_DIR=/b/c/b/mac/src/build/mac_files/Xcode.app; 
/b/c/cipd/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++
-MMD -MF obj/third_party/WebKit/Source/wtf/wtf/ThreadingPthreads.o.d
-DWTF_IMPLEMENTATION=1 -DV8_DEPRECATION_WARNINGS -DDCHECK_ALWAYS_ON=1
-DNO_TCMALLOC -DUSE_EXTERNAL_POPUP_MENU=1 -DUSE_PROPRIETARY_CODECS
-DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL
-DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED
-DCR_CLANG_REVISION=289944-2 -DCR_XCODE_VERSION=0511 -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2
-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DNDEBUG -DNVALGRIND
-DDYNAMIC_ANNOTATIONS_ENABLED=0 -DENABLE_LAYOUT_UNIT_IN_INLINE_BOXES=0
-DENABLE_OILPAN=1 -DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1
-DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION
-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -I../.. -Igen
-I../../third_party/WebKit/Source -I../../third_party/WebKit -Igen/blink
-Igen/third_party/WebKit -I../../third_party/icu/source/common
-I../../third_party/icu/source/i18n -fno-strict-aliasing -fstack-protector
-fcolor-diagnostics -arch x86_64 -Wall -Werror -Wextra -Wpartial-availability
-Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing
-Wno-covered-switch-default -Wno-deprecated-register
-Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override
-Wno-shift-negative-value -Wno-undefined-var-template
-Wno-nonportable-include-path -Wno-address-of-packed-member -O2 -g1 -isysroot
/b/c/b/mac/src/build/mac_files/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk
-mmacosx-version-min=10.9 -fvisibility=hidden -Xclang -load -Xclang
../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -Wheader-hygiene
-Wstring-conversion -Wtautological-overlap-compare -Xclang -load -Xclang
../../third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.dylib -Xclang
-add-plugin -Xclang blink-gc-plugin -Wglobal-constructors
-fno-threadsafe-statics -fvisibility-inlines-hidden -std=c++11 -stdlib=libc++
-fno-rtti -fno-exceptions -c
../../third_party/WebKit/Source/wtf/ThreadingPthreads.cpp -o
obj/third_party/WebKit/Source/wtf/wtf/ThreadingPthreads.o
../../third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:113:10: error: use of
undeclared identifier 'currentThreadSyscall'; did you mean
'internal::currentThreadSyscall'?
  return currentThreadSyscall();
         ^~~~~~~~~~~~~~~~~~~~
         internal::currentThreadSyscall
../../third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:64:18: note:
'internal::currentThreadSyscall' declared here
ThreadIdentifier currentThreadSyscall() {
                 ^

https://codereview.chromium.org/2627153004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/WTFThreadData.h (right):

https://codereview.chromium.org/2627153004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/WTFThreadData.h:56: ThreadIdentifier threadId() {
return m_threadId; }
const

[Charlie Harrison, 2017-01-12 19:39:47]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 19:40:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-01-12 19:44:33]

Yeah, I had a compile error for non TLS platforms (forgot an internal::). Should
be fine now.


I'll go ahead and land so as to unblock the thread checker as quickly as
possible. haraken: feel free to TBR.

https://codereview.chromium.org/2627153004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/WTFThreadData.h (right):

https://codereview.chromium.org/2627153004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/WTFThreadData.h:56: ThreadIdentifier threadId() {
return m_threadId; }
On 2017/01/12 19:38:49, esprehn wrote:
> const

Done.

[Charlie Harrison, 2017-01-12 19:44:53]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-01-12 19:44:53]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2627153004/#ps80001
(title: "const")

[commit-bot: I haz the power, 2017-01-12 19:45:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-01-12 19:55:45]

BTW thanks for the prompt review!

[esprehn, 2017-01-12 19:59:11]

https://codereview.chromium.org/2627153004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/WTFThreadData.cpp (right):

https://codereview.chromium.org/2627153004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/WTFThreadData.cpp:39:
m_threadId(internal::currentThreadSyscall()) {}
How does this work on non-pthreads platforms? I think you need a
currentThreadSyscall for windows?

[Charlie Harrison, 2017-01-12 20:06:45]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2017-01-12 20:07:46]

https://codereview.chromium.org/2627153004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/WTFThreadData.cpp (right):

https://codereview.chromium.org/2627153004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/WTFThreadData.cpp:39:
m_threadId(internal::currentThreadSyscall()) {}
On 2017/01/12 19:59:11, esprehn wrote:
> How does this work on non-pthreads platforms? I think you need a
> currentThreadSyscall for windows?

Good catch yeah that would lead to link errors. I've added an implementation in
ThreadingWin.

[Charlie Harrison, 2017-01-12 20:07:51]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 20:08:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-01-12 20:59:32]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2017-01-12 20:59:32]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-01-12 20:59:33]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2627153004/#ps100001
(title: "add internal::currentThreadSyscall for windows")

[commit-bot: I haz the power, 2017-01-12 20:59:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-01-12 21:27:34]

Description was changed from

==========
Use TLS storage for thread ids in wtf/ThreadingPThreads

This reduces currentThread() overhead by > ~95% on Linux (on my Z620).

BUG=TODO
==========

to

==========
Use TLS storage for thread ids in wtf/ThreadingPThreads

This reduces currentThread() overhead by > ~95% on Linux (on my Z620).

BUG=442246
==========

[Charlie Harrison, 2017-01-12 22:46:55]

Interestingly, it looks like LSAN is reporting leaks with this CL. I'm not
familiar with LSAN but I'm guessing we are somehow suppressing some of the leaks
of wtfThreadData, but this CL introduces more (on more threads).

Probably we can suppress all of these.

[esprehn, 2017-01-12 23:10:27]

On 2017/01/12 at 22:46:55, csharrison wrote:
> Interestingly, it looks like LSAN is reporting leaks with this CL. I'm not
familiar with LSAN but I'm guessing we are somehow suppressing some of the leaks
of wtfThreadData, but this CL introduces more (on more threads).
> 
> Probably we can suppress all of these.

Hmm I wonder if we actually stepped on this bug again:
https://bugs.webkit.org/show_bug.cgi?id=25973

during thread shutdown after the destruction of WTFThreadData I wonder if we're
calling wtfThreadData() again inside isMainThread() or currentThread(), which
ends up leaking a WTFThreadData for every thread?

[commit-bot: I haz the power, 2017-01-12 23:25:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 23:25:54]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-01-12 23:31:25]

You're right, I think this is (at least) a real leak where we call
isMainThread() in the destroy()  method of ThreadSpecific. I'll try to clean up
all lsan errors locally and re-upload.

Thanks, LSAN!

[haraken, 2017-01-13 04:33:44]

LGTM

https://codereview.chromium.org/2627153004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/ThreadingPthreads.cpp (right):

https://codereview.chromium.org/2627153004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:109: #else

On what platform do we hit this #else block? I think Win is using
ThreadingWin.cpp.

[haraken, 2017-01-13 04:36:07]

https://codereview.chromium.org/2627153004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/ThreadingPthreads.cpp (right):

https://codereview.chromium.org/2627153004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:106: // TLS lookup is fast
on these platforms.

Have you confirmed that TLS lookup is fast on Mac (where __GLIBC__ is enabled, I
think)?

IIRC, TLS lookup on Mac is slow and that motivated me to introduce the
performance hack in Oilpan.

[Charlie Harrison, 2017-01-13 15:56:44]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 15:57:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-01-13 16:38:45]

Ok, so I think the current PS is good enough to stop leaks. LSAN is okay
locally. Here is my reasoning:

Elliott was concerned about leaks if a String or other complex object is
destroyed as part of thread local storage, which goes ahead and calls
currentThread(). 

1. This is okay by POSIX [1]. The docs say that we first iterate over all the
destructors, which *will* end up re-allocate WTFThreadData in this case.
However, we then scan through them again (actually we scan them
PTHREAD_DESTRUCTOR_ITERATIONS times). Thus, at least for pthreads, we are safe
unless there is an infinite-recursion type thing which we originally had in the
code.

For non-pthreads, I am less sure. Comments say that we call destructors when
ThreadSpecificThreadExit is called, when threads detach. However, there are no
calls to that method anywhere according to codesearch and git grep. Could we
possibly be leaking today? I wonder if we have LSAN coverage on windows. In any
case, we could always add that second scan to ThreadSpecificThreadExit if we fix
that bug.

Here's hoping LSAN is happy :)

[1] https://linux.die.net/man/3/pthread_key_create

https://codereview.chromium.org/2627153004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/ThreadingPthreads.cpp (right):

https://codereview.chromium.org/2627153004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:106: // TLS lookup is fast
on these platforms.
On 2017/01/13 04:36:07, haraken wrote:
> 
> Have you confirmed that TLS lookup is fast on Mac (where __GLIBC__ is enabled,
I
> think)?
> 
> IIRC, TLS lookup on Mac is slow and that motivated me to introduce the
> performance hack in Oilpan.

I don't think __GLIBC__ is enabled on mac, and this #ifdef is copied directly
from ThreadState::current.

https://codereview.chromium.org/2627153004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:109: #else
On 2017/01/13 04:33:43, haraken wrote:
> 
> On what platform do we hit this #else block? I think Win is using
> ThreadingWin.cpp.
> 

I think just Mac

https://codereview.chromium.org/2627153004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/ThreadSpecific.h (right):

https://codereview.chromium.org/2627153004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/ThreadSpecific.h:48: #include "wtf/Threading.h"
Remove this.

[Charlie Harrison, 2017-01-13 16:56:55]

BTW esprehn I will wait for l-g from you before landing to double check my
reasoning.

https://codereview.chromium.org/2627153004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/ThreadSpecific.h (right):

https://codereview.chromium.org/2627153004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/ThreadSpecific.h:48: #include "wtf/Threading.h"
On 2017/01/13 16:38:45, Charlie Harrison wrote:
> Remove this.

Done.

[esprehn, 2017-01-13 17:48:46]

PTHREAD_DESTRUCTOR_ITERATIONS seems to be a small number like 4, I worry we
could end up in a situation alternating between destroying WTFThreadData and
calling isMainThread() in the destructors of thread specific datas and then we
leak them. We have that same issue today if you use create an AtomicString
inside a destructor, but that's less likely than calling isMainThread() since
it's used inside the ThreadRestrictionVerifier code and debug checks in
destructors.

I suppose lets try it though, LSAN will yell if we end up like that?

lgtm

[Charlie Harrison, 2017-01-13 17:56:37]

On 2017/01/13 17:48:46, esprehn wrote:
> PTHREAD_DESTRUCTOR_ITERATIONS seems to be a small number like 4, I worry we
> could end up in a situation alternating between destroying WTFThreadData and
> calling isMainThread() in the destructors of thread specific datas and then we
> leak them. We have that same issue today if you use create an AtomicString
> inside a destructor, but that's less likely than calling isMainThread() since
> it's used inside the ThreadRestrictionVerifier code and debug checks in
> destructors.
> 
> I suppose lets try it though, LSAN will yell if we end up like that?
> 
> lgtm

Yeah if we have any sort of infinite recursive re-entrancy we are toast, but I
think it will only happen if WTFThreadData spawns some other ThreadSpecific,
which spawns WTFThreadData again. This scopes the problem to WTFThreadData which
seems tractable. Given the previous LSAN failures I am confident LSAN will catch
that issue.

[Charlie Harrison, 2017-01-13 17:56:57]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-01-13 17:56:57]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2627153004/#ps140001
(title: "remove unnecessary include (trybots prev)")

[commit-bot: I haz the power, 2017-01-13 17:57:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-01-13 20:45:46]

FYI I am not observing a major speed up on Android with this change. I did a
simple loop for 10000 calls to WTF::internal::currentThreadSyscall and
WTF::currentThread().

It seems like the TLS approach may be slightly slower in fact on my Pixel C.

Note that this was just a one-off test using about:tracing.

Possibly, we should see if the oilpan hack makes Android faster.

[Charlie Harrison, 2017-01-13 20:48:00]

Description was changed from

==========
Use TLS storage for thread ids in wtf/ThreadingPThreads

This reduces currentThread() overhead by > ~95% on Linux (on my Z620).

BUG=442246
==========

to

==========
Use TLS storage for thread ids in wtf/ThreadingPThreads

This reduces currentThread() overhead by > ~95-99% on Linux (on my Z620).

BUG=442246
==========

[commit-bot: I haz the power, 2017-01-13 22:29:26]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484330217515340,
"parent_rev": "318867a70265ecb47c6dcbddacc153eaf34a99f9", "commit_rev":
"6deb801a3e4cd94c3a472b4e67520653afbbd380"}

[commit-bot: I haz the power, 2017-01-13 22:30:03]

Description was changed from

==========
Use TLS storage for thread ids in wtf/ThreadingPThreads

This reduces currentThread() overhead by > ~95-99% on Linux (on my Z620).

BUG=442246
==========

to

==========
Use TLS storage for thread ids in wtf/ThreadingPThreads

This reduces currentThread() overhead by > ~95-99% on Linux (on my Z620).

BUG=442246

Review-Url: https://codereview.chromium.org/2627153004
Cr-Commit-Position: refs/heads/master@{#443701}
Committed:
https://chromium.googlesource.com/chromium/src/+/6deb801a3e4cd94c3a472b4e6752...
==========

[commit-bot: I haz the power, 2017-01-13 22:30:06]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/6deb801a3e4cd94c3a472b4e6752...