Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2728453006:
WTF: Initialize main-thread stack estimates when WTF starts up. (Closed)
DescriptionWTF: Initialize main-thread stack estimates when WTF starts up.
Initializing this statically makes it sensitive to which thread it runs on for
the first time. Instead of using a static local, an ordinary global variable
is initialized once, when WTF knows it is on the main thread.
As a side effect, the check for static initialization in the function itself
is no longer required (and happens to even be safe if it is called before
initialization), which means the function need not be exported (only the
variables must be), and should compile to less code.
Finally, the method (which is useful to callers outside of WTF) is moved into
the general WTF namespace, and only the implementation details are kept in
WTF::internal.
BUG=697994
Review-Url: https://codereview.chromium.org/2728453006
Cr-Commit-Position: refs/heads/master@{#455150}
Committed: https://chromium.googlesource.com/chromium/src/+/11dcf4d494498d0a49f8a1674ad2a9f1e1f59140
Patch Set 1 #Patch Set 2 : correct call to initializeMainThreadStackEstimate #Patch Set 3 : remove internal:: prefix from ThreadSpecific #Patch Set 4 : ThreadSpecific: account for mayNotBeMainThread false positives. #Patch Set 5 : Make ThreadSpecific check for sure whether it is on the main thread. #Patch Set 6 : Don't worry about m_mainThreadStorage on platforms where we don't use it. #Patch Set 7 : an implementation of currentThread that does not use WTFThreadDAta #Patch Set 8 : fix cast for windows #
Total comments: 8
Patch Set 9 : clarify initialization of s_currentThreadKey #
Total comments: 7
Patch Set 10 : comment about why currentThread works this way #
Messages
Total messages: 56 (43 generated)
The CQ bit was checked by jbroman@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...
Description was changed from ========== WTF: Initialize main-thread stack estimates when WTF starts up. Initializing this statically makes it sensitive to which thread it runs on for the first time. Instead of using a static local, an ordinary global variable is initialized once, when WTF knows it is on the main thread. As a side effect, the check for static initialization in the function itself is no longer required (and happens to even be safe if it is called before initialization), which means the function need not be exported (only the variables must be), and should compile to less code. ========== to ========== WTF: Initialize main-thread stack estimates when WTF starts up. Initializing this statically makes it sensitive to which thread it runs on for the first time. Instead of using a static local, an ordinary global variable is initialized once, when WTF knows it is on the main thread. As a side effect, the check for static initialization in the function itself is no longer required (and happens to even be safe if it is called before initialization), which means the function need not be exported (only the variables must be), and should compile to less code. Finally, the method (which is useful to callers outside of WTF) is moved into the general WTF namespace, and only the implementation details are kept in WTF::internal. ==========
The CQ bit was checked by jbroman@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 checked by jbroman@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...
Description was changed from ========== WTF: Initialize main-thread stack estimates when WTF starts up. Initializing this statically makes it sensitive to which thread it runs on for the first time. Instead of using a static local, an ordinary global variable is initialized once, when WTF knows it is on the main thread. As a side effect, the check for static initialization in the function itself is no longer required (and happens to even be safe if it is called before initialization), which means the function need not be exported (only the variables must be), and should compile to less code. Finally, the method (which is useful to callers outside of WTF) is moved into the general WTF namespace, and only the implementation details are kept in WTF::internal. ========== to ========== WTF: Initialize main-thread stack estimates when WTF starts up. Initializing this statically makes it sensitive to which thread it runs on for the first time. Instead of using a static local, an ordinary global variable is initialized once, when WTF knows it is on the main thread. As a side effect, the check for static initialization in the function itself is no longer required (and happens to even be safe if it is called before initialization), which means the function need not be exported (only the variables must be), and should compile to less code. Finally, the method (which is useful to callers outside of WTF) is moved into the general WTF namespace, and only the implementation details are kept in WTF::internal. BUG=697994 ==========
jbroman@chromium.org changed reviewers: + yutak@chromium.org
Adithya and I ran into this as he's looking to use this trick to replace another trick in bindings (as a way of telling that we're on the main thread).
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Hmm. Non Linux seems to be having issues, will investigate.
The CQ bit was checked by jbroman@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: 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_...)
The CQ bit was checked by jbroman@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 checked by jbroman@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: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jbroman@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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by jbroman@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.
OK. The certain isMainThread check itself depended on WTF::ThreadSpecific::operator*, which caused a loop. I've resolved that by making currentThread(), which doesn't need the full sophistication of WTF::ThreadSpecific, use the underlying primitives instead and ensured that it is initialized before the rest of the threading infrastructure. Apologies for the duplication between ThreadingWin and ThreadingPthreads. I intend to follow up and deduplicate that, along with other code that is identical between the two files.
haraken@chromium.org changed reviewers: + haraken@chromium.org
It's unfortunate that we're making the ThreadSpecific logic even more complicated, but I don't have any alternative idea. https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/ThreadSpecific.h (right): https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/ThreadSpecific.h:291: if (!mainThreadAlwaysChecksTLS && UNLIKELY(ptr != &m_mainThreadStorage) && This code is in a slow path. So I'm okay with dropping the mainThreadAlwaysChecksTLS check for simplicity. https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/ThreadSpecific.h:293: m_mainThreadStorage = *ptr; Just to confirm: |*ptr| populates the thread-local storage, right? https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/Threading.h (right): https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/Threading.h:50: // Must be called before |currentThread|. Can we add a dcheck about this fact? BTW, who calls initializeCurrentThread on non-main threads? https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/WTF.cpp (right): https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/WTF.cpp:68: initializeCurrentThread(); Given that only the main thread calls this, shall we rename it to initializeMainThread?
The CQ bit was checked by jbroman@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...
https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/ThreadSpecific.h (right): https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/ThreadSpecific.h:291: if (!mainThreadAlwaysChecksTLS && UNLIKELY(ptr != &m_mainThreadStorage) && On 2017/03/06 at 01:44:03, haraken wrote: > This code is in a slow path. So I'm okay with dropping the mainThreadAlwaysChecksTLS check for simplicity. Even though |m_mainThreadStorage| will never be read? https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/ThreadSpecific.h:293: m_mainThreadStorage = *ptr; On 2017/03/06 at 01:44:03, haraken wrote: > Just to confirm: |*ptr| populates the thread-local storage, right? |ptr| either points to |offThreadPtr| (in this stack frame) or to |m_mainThreadStorage| (a member of ThreadSpecific<T>). In this branch it always points to |offThreadPtr|, so I could equivalently have written "m_mainThreadStorage = offThreadPtr;", but this seemed to me to make the intent clearer. The thread-local storage is populated by the call to ThreadSpecific::set, below. https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/Threading.h (right): https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/Threading.h:50: // Must be called before |currentThread|. On 2017/03/06 at 01:44:03, haraken wrote: > Can we add a dcheck about this fact? Sure. > BTW, who calls initializeCurrentThread on non-main threads? Nobody. This just does the one-time initialization needed: a pthread_key_t (or its Windows equivalent) needs to be created before thread-local lookups can be done, so it just needs to be done on some thread once before currentThread is called. currentThread could do this internally (and create the key on first use, assuming that it will be before a thread race exists), but this seems clearer and has fewer branches in the hot path. https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/WTF.cpp (right): https://codereview.chromium.org/2728453006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/WTF.cpp:68: initializeCurrentThread(); On 2017/03/06 at 01:44:03, haraken wrote: > Given that only the main thread calls this, shall we rename it to initializeMainThread? It only initializes the one aspect of the main thread. Namely, it initializes the code necessary for WTF::currentThread to work. (Really, I mean that it's initializing the "currentThread" function, not that it's initializing the thread itself.) I'm open to a better name. I will, at least, add a comment at the function declaration.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly looks good % small questions. https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/ThreadSpecific.h (right): https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/ThreadSpecific.h:293: m_mainThreadStorage = *ptr; Or is there any option to populate m_mainThreadStorage when initializing the main thread? Will it mess up the code? https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/ThreadingPthreads.cpp (left): https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:89: return wtfThreadData().threadId(); A stupid question: Why can't we use wtfThreadData() whereas other places are using wtfThreadData()?
https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/ThreadSpecific.h (right): https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/ThreadSpecific.h:293: m_mainThreadStorage = *ptr; On 2017/03/06 at 19:21:36, haraken wrote: > Or is there any option to populate m_mainThreadStorage when initializing the main thread? Will it mess up the code? We have no registry of all ThreadSpecific objects, and some of them require other parts of Blink to be initialized (e.g. many are Persistent handles into the Oilpan heap). Some of these objects may even allocate resources when an instance per thread is created, so I don't think it would be a good blanket policy to initialize all of them on thread start. As you say, this code is only hit when the object does not yet exist on the calling thread and so isn't hot. We could possible out-of-line this at some point if it becomes a code size issue, but most of these checks should be pretty small, and isMainThread is already out-of-line. https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/ThreadingPthreads.cpp (left): https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:89: return wtfThreadData().threadId(); On 2017/03/06 at 19:21:36, haraken wrote: > A stupid question: Why can't we use wtfThreadData() whereas other places are using wtfThreadData()? Cyclic dependency. If wtfThreadData() is used here, we can loop like this (until stack overflow): currentThread -> wtfThreadData -> ThreadSpecific::operator* -> isMainThread -> currentThread There are a few ways of resolving this cycle, of which this seemed simplest to me (especially storing the thread ID doesn't require supporting destructors etc.). It also happens to remove one pointer indirection from currentThread, and avoids reinitializing the atomic string table et al. if the thread ID is used during thread shutdown, which are nice side effects.
LGTM https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/ThreadSpecific.h (right): https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/ThreadSpecific.h:293: m_mainThreadStorage = *ptr; On 2017/03/06 20:28:09, jbroman wrote: > On 2017/03/06 at 19:21:36, haraken wrote: > > Or is there any option to populate m_mainThreadStorage when initializing the > main thread? Will it mess up the code? > > We have no registry of all ThreadSpecific objects, and some of them require > other parts of Blink to be initialized (e.g. many are Persistent handles into > the Oilpan heap). Some of these objects may even allocate resources when an > instance per thread is created, so I don't think it would be a good blanket > policy to initialize all of them on thread start. > > As you say, this code is only hit when the object does not yet exist on the > calling thread and so isn't hot. We could possible out-of-line this at some > point if it becomes a code size issue, but most of these checks should be pretty > small, and isMainThread is already out-of-line. Ah, sorry, you're totally right. Given that ThreadSpecific exists per TLS, it's never a good idea to initialize it eagerly. https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/ThreadingPthreads.cpp (left): https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:89: return wtfThreadData().threadId(); On 2017/03/06 20:28:09, jbroman wrote: > On 2017/03/06 at 19:21:36, haraken wrote: > > A stupid question: Why can't we use wtfThreadData() whereas other places are > using wtfThreadData()? > > Cyclic dependency. If wtfThreadData() is used here, we can loop like this (until > stack overflow): > > currentThread -> wtfThreadData -> ThreadSpecific::operator* -> isMainThread -> > currentThread > > There are a few ways of resolving this cycle, of which this seemed simplest to > me (especially storing the thread ID doesn't require supporting destructors > etc.). It also happens to remove one pointer indirection from currentThread, and > avoids reinitializing the atomic string table et al. if the thread ID is used > during thread shutdown, which are nice side effects. Ah, got it. Maybe it's worth having a comment :)
The CQ bit was checked by jbroman@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jbroman@chromium.org
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/2728453006/#ps180001 (title: "comment about why currentThread works this way")
https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/ThreadingPthreads.cpp (left): https://codereview.chromium.org/2728453006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:89: return wtfThreadData().threadId(); On 2017/03/06 at 20:46:42, haraken wrote: > On 2017/03/06 20:28:09, jbroman wrote: > > On 2017/03/06 at 19:21:36, haraken wrote: > > > A stupid question: Why can't we use wtfThreadData() whereas other places are > > using wtfThreadData()? > > > > Cyclic dependency. If wtfThreadData() is used here, we can loop like this (until > > stack overflow): > > > > currentThread -> wtfThreadData -> ThreadSpecific::operator* -> isMainThread -> > > currentThread > > > > There are a few ways of resolving this cycle, of which this seemed simplest to > > me (especially storing the thread ID doesn't require supporting destructors > > etc.). It also happens to remove one pointer indirection from currentThread, and > > avoids reinitializing the atomic string table et al. if the thread ID is used > > during thread shutdown, which are nice side effects. > > Ah, got it. Maybe it's worth having a comment :) Done.
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": 180001, "attempt_start_ts": 1488902588050140,
"parent_rev": "0b1f71dfa6f7dc388e7b7bec334c030b9d203e6e", "commit_rev":
"11dcf4d494498d0a49f8a1674ad2a9f1e1f59140"}
Message was sent while issue was closed.
Description was changed from ========== WTF: Initialize main-thread stack estimates when WTF starts up. Initializing this statically makes it sensitive to which thread it runs on for the first time. Instead of using a static local, an ordinary global variable is initialized once, when WTF knows it is on the main thread. As a side effect, the check for static initialization in the function itself is no longer required (and happens to even be safe if it is called before initialization), which means the function need not be exported (only the variables must be), and should compile to less code. Finally, the method (which is useful to callers outside of WTF) is moved into the general WTF namespace, and only the implementation details are kept in WTF::internal. BUG=697994 ========== to ========== WTF: Initialize main-thread stack estimates when WTF starts up. Initializing this statically makes it sensitive to which thread it runs on for the first time. Instead of using a static local, an ordinary global variable is initialized once, when WTF knows it is on the main thread. As a side effect, the check for static initialization in the function itself is no longer required (and happens to even be safe if it is called before initialization), which means the function need not be exported (only the variables must be), and should compile to less code. Finally, the method (which is useful to callers outside of WTF) is moved into the general WTF namespace, and only the implementation details are kept in WTF::internal. BUG=697994 Review-Url: https://codereview.chromium.org/2728453006 Cr-Commit-Position: refs/heads/master@{#455150} Committed: https://chromium.googlesource.com/chromium/src/+/11dcf4d494498d0a49f8a1674ad2... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/11dcf4d494498d0a49f8a1674ad2...
Message was sent while issue was closed.
On 2017/03/07 at 18:38:51, commit-bot wrote: > Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/11dcf4d494498d0a49f8a1674ad2... Also fixes crbug.com/698303.
Message was sent while issue was closed.
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
Message was sent while issue was closed.
Just noticed this CL. Thanks for fixing the bugs I caused :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
