|
|
Created:
3 years, 11 months ago by Charlie Harrison Modified:
3 years, 10 months ago Reviewers:
esprehn CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-wtf_chromium.org, chromium-reviews, dglazkov+blink, kinuko+watch, Mikhail, scheduler-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid checking for WTFThreadData::staticData in wtfThreadData()
This patch initialized the underlying ThreadSpecific<WTFThreadData>*
in a separate code path during initialization, to avoid a branch in
wtfThreadData().
BUG=621786
Review-Url: https://codereview.chromium.org/2646003003
Cr-Commit-Position: refs/heads/master@{#447897}
Committed: https://chromium.googlesource.com/chromium/src/+/395b9f9ab8004e4a0a4eb67d80f930a57964be9c
Patch Set 1 #
Total comments: 2
Patch Set 2 : Simplify :/ #
Total comments: 2
Patch Set 3 : esprehn review #Patch Set 4 : fix bad rebase #
Messages
Total messages: 36 (19 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Hmm does pthreads always destroy all the TLS stuff backwards? So if the WTFThreadData is created first it'll always be last to be destroyed? Otherwise I think the destruction of the TheadSpecific instances at thread shutdown will crash if a destructor used an AtomicString or isMainThread(). https://codereview.chromium.org/2646003003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/ThreadSpecific.h (right): https://codereview.chromium.org/2646003003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/ThreadSpecific.h:81: void initializeOnOffThread(); OnOffThread is weirdly named, did this lose a word? :P
Your worries are sound [1]: "The order of destructor calls is unspecified if more than one destructor exists for a thread when it exits." I originally thought these would go reverse order and be fine, but I guess not. I'm not sure a good solution to this. [1] https://linux.die.net/man/3/pthread_key_create https://codereview.chromium.org/2646003003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/ThreadSpecific.h (right): https://codereview.chromium.org/2646003003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/ThreadSpecific.h:81: void initializeOnOffThread(); On 2017/01/19 23:47:45, esprehn wrote: > OnOffThread is weirdly named, did this lose a word? :P Nope, I'm just awkward at naming things :P
I can't really think of a solution that would be faster than what we have today, assuming we don't do something like disallow AtomicStrings/currentThread checks in ThreadSpecifics. The first branch to see if (!WTFThreadData::staticData) can be removed without changing any behavior though, as we can always just initialize it on the main thread.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/01/20 at 00:05:27, csharrison wrote: > I can't really think of a solution that would be faster than what we have today, assuming we don't do something like disallow AtomicStrings/currentThread checks in ThreadSpecifics. > > The first branch to see if (!WTFThreadData::staticData) can be removed without changing any behavior though, as we can always just initialize it on the main thread. Yeah I'd say if we want to do that simple thing then lets do it, but this more complex thing doesn't seem like it agrees with the design of pthreads.
Ok I've rewritten this to just do the simple thing here. Probably we can close the underlying issue after this one lands. I'll rewrite the description.
Description was changed from ========== Initialize WTFThreadData::staticData on thread initialization This patch initializes the thread local storage of WTFThreadData eagerly, rather than the current lazy behavior. This involves doing some template specialization of ThreadSpecific<WTFThreadData>, as well as threading some initializeThread() calls through Platform. The result is we simplify and optimize the thread data lookup, removing two branches from wtfThreadData(). BUG=621786 ========== to ========== Avoid checking for WTFThreadData::staticData in wtfThreadData() This patch initialized the underlying ThreadSpecific<WTFThreadData>* in a separate code path during initialization, to avoid a branch in wtfThreadData(). BUG=621786 ==========
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Note that I've already made ThreadSpecifics on the main thread leaky :) https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/ThreadSpec... Since I've removed RenderThread::Shutdown, it's now unsafe to destruct ThreadSpecifics on the main thread.
True, but this issue will potentially happen on any thread.
On 2017/01/20 13:05:39, Charlie Harrison wrote: > True, but this issue will potentially happen on any thread. Sorry, I think I'm behind. If it's not guaranteed that TLS are destructed in the reverse order, why is the current code safe in the first place? ThreadSpecific<WTFThreadData> is not guaranteed to be the last TLS that get destructed already...?
This is safe because we iterate through destructors multiple times. The lazy allocation means if WTFThreadData is needed by another destructor after it is destroyed, we just instantiate it again, and use a single extra iteration to destroy the last remaining WTFThreadData. Note that this breaks for cycles, but esprehn@ and I decided it was okay.
On 2017/01/20 13:23:26, Charlie Harrison wrote: > This is safe because we iterate through destructors multiple times. > > The lazy allocation means if WTFThreadData is needed by another destructor after > it is destroyed, we just instantiate it again, and use a single extra iteration > to destroy the last remaining WTFThreadData. > > Note that this breaks for cycles, but esprehn@ and I decided it was okay. For context, see the last few comments on https://codereview.chromium.org/2627153004
However, I think you're right if we want to optimize the "stack hack" path for WTFThreadData for main thread access. Essentially, if internal::mayNotBeMainThread() is false, I think we can unconditionally return m_mainThreadStorage, as long as the ThreadSpecific is first initialized on the main thread (where we initialize m_mainThreadStorage). We could either add another method, or templatize ThreadSpecific on that property. I am unsure if the benefits (one fewer branch, only in some platforms) outweigh the extra code though.
esprehn: FTR, I still want to land this change separately, as it is trivial, so PTAL. I'll look into doing something like #21 as a followup.
I think you want to remove that comment. Otherwise LGTM. Note that I think this means that creating an AtomicString inside a ThreadSpecific destructor would possibly crash if the WTFThreadData was destroyed already since we don't lazy allocate another one. Let's see if that ever happens... https://codereview.chromium.org/2646003003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/WTFThreadData.cpp (right): https://codereview.chromium.org/2646003003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/WTFThreadData.cpp:45: // WTFThreadData is used on main thread before it could possibly be used You can remove this comment now. It's per thread and we don't care about which one
I don't think this will cause a crash, because the global WTFThreadData ThreadSpecific will never be destructed. It is intentionally leaked. There are some comments to this effect in ~ThreadSpecific(). https://codereview.chromium.org/2646003003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/WTFThreadData.cpp (right): https://codereview.chromium.org/2646003003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/WTFThreadData.cpp:45: // WTFThreadData is used on main thread before it could possibly be used On 2017/02/02 23:24:51, esprehn wrote: > You can remove this comment now. It's per thread and we don't care about which > one Done.
The CQ bit was checked by csharrison@chromium.org
The CQ bit was unchecked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
The CQ bit was unchecked by csharrison@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by csharrison@chromium.org
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/2646003003/#ps60001 (title: "fix bad rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486078821519820, "parent_rev": "d50208a95a17148ec119745d20ceba483da71672", "commit_rev": "395b9f9ab8004e4a0a4eb67d80f930a57964be9c"}
Message was sent while issue was closed.
Description was changed from ========== Avoid checking for WTFThreadData::staticData in wtfThreadData() This patch initialized the underlying ThreadSpecific<WTFThreadData>* in a separate code path during initialization, to avoid a branch in wtfThreadData(). BUG=621786 ========== to ========== Avoid checking for WTFThreadData::staticData in wtfThreadData() This patch initialized the underlying ThreadSpecific<WTFThreadData>* in a separate code path during initialization, to avoid a branch in wtfThreadData(). BUG=621786 Review-Url: https://codereview.chromium.org/2646003003 Cr-Commit-Position: refs/heads/master@{#447897} Committed: https://chromium.googlesource.com/chromium/src/+/395b9f9ab8004e4a0a4eb67d80f9... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/395b9f9ab8004e4a0a4eb67d80f9... |