Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(555)

Issue 2646003003: Avoid checking for WTFThreadData::staticData in wtfThreadData() (Closed)

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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -6 lines) Patch
M third_party/WebKit/Source/wtf/ThreadingPthreads.cpp View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/wtf/ThreadingWin.cpp View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/wtf/WTFThreadData.h View 1 2 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/wtf/WTFThreadData.cpp View 1 2 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 36 (19 generated)
esprehn
Hmm does pthreads always destroy all the TLS stuff backwards? So if the WTFThreadData is ...
3 years, 11 months ago (2017-01-19 23:47:45 UTC) #4
Charlie Harrison
Your worries are sound [1]: "The order of destructor calls is unspecified if more than ...
3 years, 11 months ago (2017-01-19 23:54:42 UTC) #5
Charlie Harrison
I can't really think of a solution that would be faster than what we have ...
3 years, 11 months ago (2017-01-20 00:05:27 UTC) #6
esprehn
On 2017/01/20 at 00:05:27, csharrison wrote: > I can't really think of a solution that ...
3 years, 11 months ago (2017-01-20 01:04:13 UTC) #9
Charlie Harrison
Ok I've rewritten this to just do the simple thing here. Probably we can close ...
3 years, 11 months ago (2017-01-20 01:17:22 UTC) #10
haraken
Note that I've already made ThreadSpecifics on the main thread leaky :) https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/ThreadSpecific.h?q=threadspecific&sq=package:chromium&dr=CSs&l=239 Since I've ...
3 years, 11 months ago (2017-01-20 06:32:49 UTC) #16
Charlie Harrison
True, but this issue will potentially happen on any thread.
3 years, 11 months ago (2017-01-20 13:05:39 UTC) #17
haraken
On 2017/01/20 13:05:39, Charlie Harrison wrote: > True, but this issue will potentially happen on ...
3 years, 11 months ago (2017-01-20 13:21:04 UTC) #18
Charlie Harrison
This is safe because we iterate through destructors multiple times. The lazy allocation means if ...
3 years, 11 months ago (2017-01-20 13:23:26 UTC) #19
Charlie Harrison
On 2017/01/20 13:23:26, Charlie Harrison wrote: > This is safe because we iterate through destructors ...
3 years, 11 months ago (2017-01-20 13:25:07 UTC) #20
Charlie Harrison
However, I think you're right if we want to optimize the "stack hack" path for ...
3 years, 11 months ago (2017-01-20 14:53:42 UTC) #21
Charlie Harrison
esprehn: FTR, I still want to land this change separately, as it is trivial, so ...
3 years, 11 months ago (2017-01-23 16:05:15 UTC) #22
esprehn
I think you want to remove that comment. Otherwise LGTM. Note that I think this ...
3 years, 10 months ago (2017-02-02 23:24:51 UTC) #23
Charlie Harrison
I don't think this will cause a crash, because the global WTFThreadData ThreadSpecific will never ...
3 years, 10 months ago (2017-02-02 23:35:39 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2646003003/20001
3 years, 10 months ago (2017-02-02 23:36:33 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2646003003/60001
3 years, 10 months ago (2017-02-02 23:41:14 UTC) #33
commit-bot: I haz the power
3 years, 10 months ago (2017-02-03 03:22:59 UTC) #36
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/395b9f9ab8004e4a0a4eb67d80f9...

Powered by Google App Engine
This is Rietveld 408576698