Fix dead ALWAYS_INLINE path for Blink thread-local getter [chromium/src : main]
Zhou, Shuangshuang (Gerrit)
Zhou, Shuangshuang added 1 comment![]( "Open in Gerrit")
Hi Philip&Mason, PTAL!
This came out of some VTune profiling on our Intel/Windows platforms: `blink::ThreadStateStorage::Current` shows up as an out-of-line NOINLINE call even in official builds. Tracing it back, `#if defined(BLINK_HEAP_HIDE_THREAD_LOCAL_IN_LIBRARY)` is always true (the macro is always #defined, to 0 or 1), so the ALWAYS_INLINE getter that was meant for non-component builds has been dead code.
The CL switches the guard to check the macro's value, and conservatively enables the inlined getter only on Windows official builds (initial-exec — a call-free TLS load). Other platform builds keep today's out-of-line getter.
I ran 3 spd3 tests for win11-perf and 3 spd3 tests for win11-perf-pgo. Looks like there is no regression and slight performance improvement. Feel free to correct me if my understanding is wrong.
Thanks!
Related details
- Mason Freed
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
I think bikineev@ would be a better reviewer, since he [wrote the original code](https://crrev.com/c/2896895).
Related details
- Anton Bikineev
- Philip Rogers
- Zhou, Shuangshuang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev added 1 comment![]( "Open in Gerrit")
#if defined(BLINK_HEAP_HIDE_THREAD_LOCAL_IN_LIBRARY)This is a nice catch. The main question is why lifting it only on Win? Currently we have two runtime calls on Darwin - one the NOINLINE call to the getter and the other one `__tls_get_addr`.
Related details
- Philip Rogers
- Zhou, Shuangshuang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1641d603290000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15586a66290000
Zhou, Shuangshuang (Gerrit)
Zhou, Shuangshuang added 1 comment![]( "Open in Gerrit")
#if defined(BLINK_HEAP_HIDE_THREAD_LOCAL_IN_LIBRARY)This is a nice catch. The main question is why lifting it only on Win? Currently we have two runtime calls on Darwin - one the NOINLINE call to the getter and the other one `__tls_get_addr`.
Thanks Anton — nice one, and yeah, on Darwin it's two calls today.
You're right that inlining would still knock out the outer getter call there (2 → 1). The reason I kept it to Windows is really about what's left after inlining:
- On Windows (initial-exec) the getter is wrapping a call-free TLS load, so inlining gets us all the way to zero calls — just a cheap TEB-relative load left. VTune on our RVP shows the out-of-line `ThreadStateStorage::Current` symbol simply vanishing, so it's a real drop in overhead.
- On Darwin the getter is wrapping that second TLS-resolution call. Inlining drops the outer one (2 → 1), but then the whole sequence (that call + its setup) gets copied into every one of Current()'s many call sites, and I was a little wary the code-size growth could outweigh the single saved call and regress. Android's in the same boat (local-dynamic).
Currently I've kept it Windows-only so we don't risk the platforms we haven't profiled yet. Happy to flip it on for all non-component builds and run the full Pinpoint set (Win / Linux / Mac / Android), then gate out whatever actually regresses. Then we just open it with:
```
#if BLINK_HEAP_HIDE_THREAD_LOCAL_IN_LIBRARY
```
WDYT?
Thanks！
Related details
- Anton Bikineev
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhou, Shuangshuang (Gerrit)
Zhou, Shuangshuang added 1 comment![]( "Open in Gerrit")
Hi Anton, any update or idea on this?
Thanks!
Anton Bikineev (Gerrit)
Anton Bikineev voted and added 2 comments![]( "Open in Gerrit")
Votes added by Anton Bikineev
| Code-Review | +1 |
2 comments
lgtm
#if defined(BLINK_HEAP_HIDE_THREAD_LOCAL_IN_LIBRARY)Zhou, ShuangshuangThis is a nice catch. The main question is why lifting it only on Win? Currently we have two runtime calls on Darwin - one the NOINLINE call to the getter and the other one `__tls_get_addr`.
Thanks Anton — nice one, and yeah, on Darwin it's two calls today.
You're right that inlining would still knock out the outer getter call there (2 → 1). The reason I kept it to Windows is really about what's left after inlining:
- On Windows (initial-exec) the getter is wrapping a call-free TLS load, so inlining gets us all the way to zero calls — just a cheap TEB-relative load left. VTune on our RVP shows the out-of-line `ThreadStateStorage::Current` symbol simply vanishing, so it's a real drop in overhead.
- On Darwin the getter is wrapping that second TLS-resolution call. Inlining drops the outer one (2 → 1), but then the whole sequence (that call + its setup) gets copied into every one of Current()'s many call sites, and I was a little wary the code-size growth could outweigh the single saved call and regress. Android's in the same boat (local-dynamic).
Currently I've kept it Windows-only so we don't risk the platforms we haven't profiled yet. Happy to flip it on for all non-component builds and run the full Pinpoint set (Win / Linux / Mac / Android), then gate out whatever actually regresses. Then we just open it with:
```
#if BLINK_HEAP_HIDE_THREAD_LOCAL_IN_LIBRARY
```WDYT?
Thanks！
I've tried this on M1 and it actually surprisingly regressed. I'm not sure if code-size growth is concern though given the icache size. Can you add a TODO to investigate it? Thanks!
Related details
- Philip Rogers
- Zhou, Shuangshuang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
// Everywhere else the access is itself a runtime call -- Apple routes everyIs the "Everywhere else..." comment correct for ChromeOS and Linux? The table at the top of the file seems to say these platforms use "the fastest local-exec"? Should we instead just exclude Android and Mac, rather than only including windows?
Related details
- Zhou, Shuangshuang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/speedometer-main.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/146c1f98a90000
Related details
- Anton Bikineev
- Zhou, Shuangshuang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1024fb40a90000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/119dea32a90000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/speedometer-main.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/13f43588a90000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/speedometer-main.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1073f79ca90000
Zhou, Shuangshuang (Gerrit)
Zhou, Shuangshuang added 3 comments![]( "Open in Gerrit")
Hi Philip/Anton, PT-Another-L!
Thanks!
#if defined(BLINK_HEAP_HIDE_THREAD_LOCAL_IN_LIBRARY)Zhou, ShuangshuangThis is a nice catch. The main question is why lifting it only on Win? Currently we have two runtime calls on Darwin - one the NOINLINE call to the getter and the other one `__tls_get_addr`.
Anton BikineevThanks Anton — nice one, and yeah, on Darwin it's two calls today.
You're right that inlining would still knock out the outer getter call there (2 → 1). The reason I kept it to Windows is really about what's left after inlining:
- On Windows (initial-exec) the getter is wrapping a call-free TLS load, so inlining gets us all the way to zero calls — just a cheap TEB-relative load left. VTune on our RVP shows the out-of-line `ThreadStateStorage::Current` symbol simply vanishing, so it's a real drop in overhead.
- On Darwin the getter is wrapping that second TLS-resolution call. Inlining drops the outer one (2 → 1), but then the whole sequence (that call + its setup) gets copied into every one of Current()'s many call sites, and I was a little wary the code-size growth could outweigh the single saved call and regress. Android's in the same boat (local-dynamic).
Currently I've kept it Windows-only so we don't risk the platforms we haven't profiled yet. Happy to flip it on for all non-component builds and run the full Pinpoint set (Win / Linux / Mac / Android), then gate out whatever actually regresses. Then we just open it with:
```
#if BLINK_HEAP_HIDE_THREAD_LOCAL_IN_LIBRARY
```WDYT?
Thanks！
I've tried this on M1 and it actually surprisingly regressed. I'm not sure if code-size growth is concern though given the icache size. Can you add a TODO to investigate it? Thanks!
Done!
// Everywhere else the access is itself a runtime call -- Apple routes everyIs the "Everywhere else..." comment correct for ChromeOS and Linux? The table at the top of the file seems to say these platforms use "the fastest local-exec"? Should we instead just exclude Android and Mac, rather than only including windows?
I think Yes, it should just exclude Android and Apple rather than only including Windows. Linux/ChromeOS resolve to local-exec in non-component builds, same call-free access as Windows' initial-exec, so there's no mechanism reason to leave them on the slow path. Now only Android and Mac is exluded.
Thanks!
Related details
- Anton Bikineev
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
1 comment
- Anton Bikineev
- Zhou, Shuangshuang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anton Bikineev
| Code-Review | +1 |
1 comment
- Zhou, Shuangshuang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhou, Shuangshuang (Gerrit)
Zhou, Shuangshuang voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix dead ALWAYS_INLINE path for Blink thread-local getter
BLINK_HEAP_HIDE_THREAD_LOCAL_IN_LIBRARY is always #defined -- to 1 in
component (shared-library) builds and to 0 otherwise (see
thread_local.h). So the getter selection
#if defined(BLINK_HEAP_HIDE_THREAD_LOCAL_IN_LIBRARY)
always evaluated true and unconditionally selected the NOINLINE
out-of-line getter. The #else branch, which defines the intended
ALWAYS_INLINE getter for non-component builds, has therefore been dead
code on every platform and configuration since it was added -- even
though the sibling TLS-model selection just above already switches on
the macro's value and assumes non-component builds access the
thread_local directly.
Check the macro's value instead of its definedness, and enable the
inlined getter where the platform's TLS model resolves the variable with
a call-free static access: non-component builds on Windows
("initial-exec") and Linux/ChromeOS ("local-exec"). Component builds,
Apple and Android keep the out-of-line NOINLINE getter exactly as they
ship today -- their access is itself a runtime call (Apple routes every
thread_local through Darwin's _tlv_get_addr thunk regardless of
tls_model; Android and component builds use "local-dynamic", a
__tls_get_addr call), so there is no call-free load for inlining to
expose. Inlining on Apple would still fold away the outer getter call,
but it measured a regression on an M1; a TODO tracks investigating that
separately.
Behavior observed with VTune on an official Windows 11 build (Chrome
152.0.7962.0, PGO and ThinLTO enabled), for the getter symbol
(out-of-line body in thread_state_storage.cc):
blink::ThreadStateStorage::Current | Before (default) | After (fix)
-----------------------------------+------------------+-------------
Clockticks | 51,604,000 | not present
Instructions Retired | 70,034,000 | not present
CPI Rate | 0.737 | n/a
Before the fix the out-of-line symbol is present, confirming the dead
NOINLINE selection is what ships. After the fix the symbol is gone with
no replacement out-of-line helper, confirming the getter is inlined into
its callers as the #else branch always intended. The thread_local load
itself is unchanged; only the call boundary and the NOINLINE barrier are
removed.
- M third_party/blink/renderer/platform/heap/thread_local.h
Code-Review: +1 by Anton Bikineev, +1 by Philip Rogers
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |