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_compile_dbg_ng/builds/336547)
3 years, 11 months ago
(2017-01-12 21:00:51 UTC)
#4
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/193529)
3 years, 11 months ago
(2017-01-13 21:41:53 UTC)
#8
Description was changed from ========== [WIP] Fast path for currentThread() for main thread on TLS-slow ...
3 years, 11 months ago
(2017-01-13 23:18:06 UTC)
#11
Description was changed from
==========
[WIP] Fast path for currentThread() for main thread on TLS-slow platforms
BUG=
==========
to
==========
[WIP] Fast path for currentThread() for main thread on TLS-slow platforms
This code is mainly a refactor of utility code out of platform/heap and
into wtf. WTFThreadData then copies some tricks done by ThreadState.
BUG=
==========
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 11 months ago
(2017-01-14 00:49:15 UTC)
#12
3 years, 11 months ago
(2017-01-14 00:49:16 UTC)
#13
Dry run: This issue passed the CQ dry run.
Charlie Harrison
Description was changed from ========== [WIP] Fast path for currentThread() for main thread on TLS-slow ...
3 years, 11 months ago
(2017-01-15 06:37:12 UTC)
#14
Description was changed from
==========
[WIP] Fast path for currentThread() for main thread on TLS-slow platforms
This code is mainly a refactor of utility code out of platform/heap and
into wtf. WTFThreadData then copies some tricks done by ThreadState.
BUG=
==========
to
==========
[WIP] Fast path for currentThread() for main thread on TLS-slow platforms
This code is mainly a refactor of utility code out of platform/heap and
into wtf. WTFThreadData then copies some tricks done by ThreadState.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
Charlie Harrison
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
3 years, 11 months ago
(2017-01-15 06:47:59 UTC)
#15
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/369352)
3 years, 11 months ago
(2017-01-15 07:16:25 UTC)
#18
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/348707)
3 years, 11 months ago
(2017-01-15 20:16:33 UTC)
#22
3 years, 11 months ago
(2017-01-16 00:51:51 UTC)
#26
Dry run: This issue passed the CQ dry run.
Charlie Harrison
Description was changed from ========== [WIP] Fast path for currentThread() for main thread on TLS-slow ...
3 years, 11 months ago
(2017-01-16 02:44:18 UTC)
#27
Description was changed from
==========
[WIP] Fast path for currentThread() for main thread on TLS-slow platforms
This code is mainly a refactor of utility code out of platform/heap and
into wtf. WTFThreadData then copies some tricks done by ThreadState.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Fast path for currentThread() for main thread on TLS-slow platforms
This code is mainly a refactor of utility code out of platform/heap and
into wtf. WTFThreadData then copies some tricks done by ThreadState.
BUG=TODO
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
Hey haraken@, This is the stack based optimization ported from ThreadState we discussed earlier. Most ...
3 years, 11 months ago
(2017-01-16 02:44:40 UTC)
#29
Hey haraken@,
This is the stack based optimization ported from ThreadState we discussed
earlier. Most of the logic is unchanged and moved from platform/heap to wtf
unchanged.
I'm hoping this will further reduce overhead from the thread checker (and
isMainThread() / currentThread()) for platforms where TLS is slow.
Note that there are some subtleties with initialization order (esp. with MSVC),
so please let me know if you think I should simplify.
Charlie Harrison
Oh, and let me know if this is what you had in mind for crbug.com/621786. ...
3 years, 11 months ago
(2017-01-16 03:25:33 UTC)
#30
Oh, and let me know if this is what you had in mind for crbug.com/621786. Were
you thinking that we would have this fast path for all ThreadSpecifics via some
growable static buffer?
haraken
On 2017/01/16 03:25:33, Charlie Harrison wrote: > Oh, and let me know if this is ...
3 years, 11 months ago
(2017-01-16 04:42:30 UTC)
#31
Description was changed from ========== Fast path for currentThread() for main thread on TLS-slow platforms ...
3 years, 11 months ago
(2017-01-17 21:58:57 UTC)
#34
Description was changed from
==========
Fast path for currentThread() for main thread on TLS-slow platforms
This code is mainly a refactor of utility code out of platform/heap and
into wtf. WTFThreadData then copies some tricks done by ThreadState.
BUG=TODO
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Fast path for currentThread() for main thread on TLS-slow platforms
This code is mainly a refactor of utility code out of platform/heap and
into wtf/StackUtil. ThreadSpecific then copies some tricks done by
ThreadState.
BUG=621786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
Charlie Harrison
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
3 years, 11 months ago
(2017-01-17 22:07:22 UTC)
#35
Description was changed from ========== Fast path for currentThread() for main thread on TLS-slow platforms ...
3 years, 11 months ago
(2017-01-17 22:21:06 UTC)
#39
Description was changed from
==========
Fast path for currentThread() for main thread on TLS-slow platforms
This code is mainly a refactor of utility code out of platform/heap and
into wtf/StackUtil. ThreadSpecific then copies some tricks done by
ThreadState.
BUG=621786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Fast path for ThreadSpecific for main thread on TLS-slow platforms
This code is mainly a refactor of utility code out of platform/heap and
into wtf/StackUtil. ThreadSpecific then copies some tricks done by
ThreadState.
BUG=621786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 11 months ago
(2017-01-17 22:40:50 UTC)
#40
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/349649)
3 years, 11 months ago
(2017-01-17 22:40:51 UTC)
#41
Hey haraken@, Would you take another look? This change tries to constrain most of the ...
3 years, 11 months ago
(2017-01-18 01:49:48 UTC)
#50
Hey haraken@,
Would you take another look? This change tries to constrain most of the logic to
the new StackUtil file, while still caching the stack size in WTFThreadData.
https://codereview.chromium.org/2623273007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/ThreadState.h (right):
https://codereview.chromium.org/2623273007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/ThreadState.h:189:
DCHECK_EQ(**s_threadSpecific, mainThreadState());
On 2017/01/16 04:42:30, haraken wrote:
>
> Ideally s_threadSpecific should be moved into WTFThreadData. Then we can just
> use WTF::WTFThreadData::isMainThread() here. We don't need to expose
> WTFThreadData::stackBasedIsMainThread.
I've just turned this into **s_threadSpecific because now all the logic is in
ThreadSpecific.h.
https://codereview.chromium.org/2623273007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/WTFThreadData.cpp (right):
https://codereview.chromium.org/2623273007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/WTFThreadData.cpp:54: }
On 2017/01/16 04:42:30, haraken wrote:
>
> Can we do this initialization in WTFThreadData::initialize()? Then
WTFThreadData
> doesn't need to take a |type| parameter.
This is all gone.
https://codereview.chromium.org/2623273007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/WTFThreadData.cpp:63: size_t
WTFThreadData::threadStackSize() {
On 2017/01/16 04:42:30, haraken wrote:
>
> I want to put this method in the same file as getUnderestimatedStackSize().
Can
> we move the method to StackUtil.cpp? (Or move methods in StackUtil.cpp to this
> file?)
Done.
https://codereview.chromium.org/2623273007/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/WTFThreadData.h (right):
https://codereview.chromium.org/2623273007/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/WTFThreadData.h:71: static bool
stackBasedIsMainThread();
On 2017/01/16 04:42:30, haraken wrote:
>
> Let's mention that this may return false even if it's the main thread.
>
> To clarify the semantics, we might want to rename it to mayNotBeMainThread and
> flip the returned true/false.
Done.
Charlie Harrison
Hmm still seeing test failures on Windows. I'll try to build locally tomorrow :/
3 years, 11 months ago
(2017-01-18 02:45:50 UTC)
#51
Hmm still seeing test failures on Windows. I'll try to build locally tomorrow :/
Charlie Harrison
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
3 years, 11 months ago
(2017-01-18 03:34:28 UTC)
#52
Thanks, tried to address all your concerns. https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Source/wtf/StackUtil.cpp File third_party/WebKit/Source/wtf/StackUtil.cpp (right): https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Source/wtf/StackUtil.cpp#newcode25 third_party/WebKit/Source/wtf/StackUtil.cpp:25: reinterpret_cast<uintptr_t>(WTF::getStackStart()) - ...
3 years, 11 months ago
(2017-01-18 16:29:55 UTC)
#62
Thanks, tried to address all your concerns.
https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/StackUtil.cpp (right):
https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/StackUtil.cpp:25:
reinterpret_cast<uintptr_t>(WTF::getStackStart()) - sizeof(void*);
On 2017/01/18 05:29:42, haraken wrote:
>
> Add a comment why we're subtracting sizeof(void*).
I had to look at historical blame to figure this one out. I looks like
getStackStart is exclusive, not inclusive (e.g. it points past
the last page of the stack in linear order), so we subtract sizeof(void*) here
and do a >= comparison with this value.
https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/StackUtil.cpp:35: s_underestimatedStackSize =
underestimatedStackSize - sizeof(void*);
On 2017/01/18 05:29:42, haraken wrote:
>
> Ditto.
Done.
https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/StackUtil.cpp:167: bool mayNotBeMainThread() {
On 2017/01/18 05:29:42, haraken wrote:
>
> Can we move this function (and all functions called in this function) to a
> header file? It's important that the function is inlined.
Done, though I haven't inlined the initializers to the static variables which
will only be called once.
https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/ThreadSpecific.h (right):
https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/ThreadSpecific.h:283: if (UNLIKELY(!*ptr)) {
On 2017/01/18 05:29:42, haraken wrote:
>
> Not related to this CL but we might want to remove the logic to instantiate
> ThreadSpecific lazily. Instead we should just instantiate ThreadSpecific
eagerly
> when a thread gets initialized. Then we won't need the branch.
>
Acknowledged, though I wonder if that would lead to slower thread initialization
for cases when we certainly don't care about a ThreadSpecific except on one or
two threads?
https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/ThreadSpecific.h:283: if (UNLIKELY(!*ptr)) {
On 2017/01/18 05:29:42, haraken wrote:
>
> Can we just use if(!offThreadPtr) ? I don't fully understand why we need
|ptr|.
>
We use pointer indirection here because ptr can point to offThreadPtr *or*
m_mainThreadStorage, and either can be uninitialized. This allows us to use the
same code path for the main thread case and the off thread case without
duplicating something like an inline method for "init value".
We can probably remove this if your comment below is addressed.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 11 months ago
(2017-01-18 18:22:37 UTC)
#63
3 years, 11 months ago
(2017-01-18 18:22:41 UTC)
#64
Dry run: This issue passed the CQ dry run.
Charlie Harrison
Because I was not able to address all your comments, I'll wait for one more ...
3 years, 11 months ago
(2017-01-18 21:41:48 UTC)
#65
Because I was not able to address all your comments, I'll wait for one more l-g
before landing.
haraken
LGTM https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Source/wtf/ThreadSpecific.h File third_party/WebKit/Source/wtf/ThreadSpecific.h (right): https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Source/wtf/ThreadSpecific.h#newcode283 third_party/WebKit/Source/wtf/ThreadSpecific.h:283: if (UNLIKELY(!*ptr)) { On 2017/01/18 16:29:55, Charlie Harrison ...
3 years, 11 months ago
(2017-01-19 01:04:39 UTC)
#66
LGTM
https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/ThreadSpecific.h (right):
https://codereview.chromium.org/2623273007/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/ThreadSpecific.h:283: if (UNLIKELY(!*ptr)) {
On 2017/01/18 16:29:55, Charlie Harrison wrote:
> On 2017/01/18 05:29:42, haraken wrote:
> >
> > Can we just use if(!offThreadPtr) ? I don't fully understand why we need
> |ptr|.
> >
>
> We use pointer indirection here because ptr can point to offThreadPtr *or*
> m_mainThreadStorage, and either can be uninitialized. This allows us to use
the
> same code path for the main thread case and the off thread case without
> duplicating something like an inline method for "init value".
>
> We can probably remove this if your comment below is addressed.
Thread initialization is already heavy, so I don't really care about the
overhead :)
Also it sounds like a good idea to have different methods for a main thread case
and a non-main thread case, which enables us to remove the if branch from the
main thread case.
You can address it in a follow-up.
Charlie Harrison
Thank you, haraken!
3 years, 11 months ago
(2017-01-19 01:05:49 UTC)
#67
Thank you, haraken!
Charlie Harrison
The CQ bit was checked by csharrison@chromium.org
3 years, 11 months ago
(2017-01-19 01:06:02 UTC)
#68
CQ is committing da patch. Bot data: {"patchset_id": 270001, "attempt_start_ts": 1484787962638790, "parent_rev": "4a3f02d18631fc413399a5e058b89f34628826d9", "commit_rev": "098f72afae1133c762650b2c841981be9b4afe6e"}
3 years, 11 months ago
(2017-01-19 01:12:42 UTC)
#70
CQ is committing da patch.
Bot data: {"patchset_id": 270001, "attempt_start_ts": 1484787962638790,
"parent_rev": "4a3f02d18631fc413399a5e058b89f34628826d9", "commit_rev":
"098f72afae1133c762650b2c841981be9b4afe6e"}
commit-bot: I haz the power
Description was changed from ========== Fast path for ThreadSpecific for main thread on TLS-slow platforms ...
3 years, 11 months ago
(2017-01-19 01:13:13 UTC)
#71
Message was sent while issue was closed.
Description was changed from
==========
Fast path for ThreadSpecific for main thread on TLS-slow platforms
This code is mainly a refactor of utility code out of platform/heap and
into wtf/StackUtil. ThreadSpecific then copies some tricks done by
ThreadState.
BUG=621786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Fast path for ThreadSpecific for main thread on TLS-slow platforms
This code is mainly a refactor of utility code out of platform/heap and
into wtf/StackUtil. ThreadSpecific then copies some tricks done by
ThreadState.
BUG=621786
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2623273007
Cr-Commit-Position: refs/heads/master@{#444581}
Committed:
https://chromium.googlesource.com/chromium/src/+/098f72afae1133c762650b2c8419...
==========
commit-bot: I haz the power
Committed patchset #15 (id:270001) as https://chromium.googlesource.com/chromium/src/+/098f72afae1133c762650b2c841981be9b4afe6e
3 years, 11 months ago
(2017-01-19 01:13:15 UTC)
#72
Issue 2623273007: Fast path for ThreadSpecific for main thread on TLS-slow platforms
(Closed)
Created 3 years, 11 months ago by Charlie Harrison
Modified 3 years, 11 months ago
Reviewers: haraken
Base URL:
Comments: 20
Chromium Code Reviews
Issue
2623273007:
Fast path for ThreadSpecific for main thread on TLS-slow platforms (Closed)
