spectre mitigation speculation barriers are still used in generateVMWrappers
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
People
(Reporter: jrmuizel, Assigned: jrmuizel, NeedInfo)
References
(Blocks 1 open bug)
Details
(Keywords: perf-alert, Whiteboard: [sp3])
Attachments
(1 file)
|
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
I think this is unintentional and seems to be showing up pretty prominently on M1 macs
|
Assignee | |
Comment 1•3 months ago
|
||
js::jit::MacroAssembler::speculationBarrier() [/Users/mstange/code/mozilla/js/src/jit/arm64/MacroAssembler-arm64.cpp]
js::jit::JitRuntime::generateVMWrapper(JSContext*, js::jit::MacroAssembler&, js::jit::VMFunctionId, js::jit::VMFunctionData const&, js::jit::DynFn, unsigned int*) [/Users/mstange/code/mozilla/js/src/jit/arm64/Trampoline-arm64.cpp]
js::jit::JitRuntime::generateVMWrappers(JSContext*, js::jit::MacroAssembler&, js::jit::PerfSpewerRangeRecorder&) [/Users/mstange/code/mozilla/js/src/jit/VMFunctions.cpp]
js::jit::JitRuntime::generateTrampolines(JSContext*) [/Users/mstange/code/mozilla/js/src/jit/Ion.cpp]
js::jit::JitRuntime::initialize(JSContext*) [/Users/mstange/code/mozilla/js/src/jit/Ion.cpp]
JSRuntime::createJitRuntime(JSContext*) [/Users/mstange/code/mozilla/js/src/vm/Realm.cpp]
JS::InitSelfHostedCode(JSContext*, mozilla::Span<unsigned char const, (unsigned long)18446744073709551615>, bool (*)(JSContext*, mozilla::Span<unsigned char const, (unsigned long)18446744073709551615>)) [/Users/mstange/code/mozilla/js/src/vm/Initialization.cpp]
XPCJSContext::Initialize() [/Users/mstange/code/mozilla/js/xpconnect/src/XPCJSContext.cpp]
XPCJSContext::NewXPCJSContext() [/Users/mstange/code/mozilla/js/xpconnect/src/XPCJSContext.cpp]
nsXPConnect::InitJSContext() [/Users/mstange/code/mozilla/js/xpconnect/src/nsXPConnect.cpp]
xpc::InitializeJSContext() [/Users/mstange/code/mozilla/js/xpconnect/src/nsXPConnect.cpp]
NS_InitXPCOM [/Users/mstange/code/mozilla/xpcom/build/XPCOMInit.cpp]
mozilla::dom::ContentProcess::Init(int, char**) [/Users/mstange/code/mozilla/dom/ipc/ContentProcess.cpp]
XRE_InitChildProcess(int, char**, XREChildData const*) [/Users/mstange/code/mozilla/toolkit/xre/nsEmbedFunctions.cpp]
XRE_InitChildProcess
content_process_main(mozilla::Bootstrap*, int, char**) [/Users/mstange/code/mozilla/ipc/app/../contentproc/plugin-container.cpp]
main [/Users/mstange/code/mozilla/ipc/app/MozillaRuntimeMain.cpp]
|
|
Assignee | |
Comment 2•3 months ago
|
||
We want this disabled early so that it's not used in generateVMWrapper
during initialization.
This mitigation has only theoretical value and a very real performance
cost.
|
||
Updated•3 months ago
|
|
|
Assignee | |
Updated•3 months ago
|
|
|
Assignee | |
Comment 3•3 months ago
|
||
|
||
Updated•3 months ago
|
|
|
Assignee | |
Comment 4•3 months ago
|
||
Looks like the engine default is being clobbered by https://searchfox.org/mozilla-central/rev/2c3d657cbba5484ccac44443c4417baed7b5fafb/modules/libpref/init/StaticPrefList.yaml#7725 so my patch had no effect.
|
|
Assignee | |
Comment 5•3 months ago
|
||
Here are profiles of the todos function from react-redux:
before: https://share.firefox.dev/3Shwwhn (272 samples)
after: https://share.firefox.dev/49hGKFa (230 samples)
Pushed by jmuizelaar@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dabc78ed3644 Disable spectreJitToCxxCalls everywhere. r=jandem
|
|
Assignee | |
Comment 7•3 months ago
|
||
Comment on attachment 9377326 [details]
Bug 1877605. Disable spectreJitToCxxCalls everywhere.
Beta/Release Uplift Approval Request
- User impact if declined: 2.8% on Speedometer3 on ARM64 macOS
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This is effectively just a pref change. We already expected it to be off it just wasn't working properly.
- String changes made/needed:
- Is Android affected?: Yes
|
|
Assignee | |
Comment 8•3 months ago
•
|
||
Try results suggest that this is a 2.8% improvement on ARM64 macOS
|
||
Comment 9•3 months ago
|
||
Backed out for causing mochitest failures at js/xpconnect/tests/mochitest/test_spectre_mitigations.html
Backout link: https://hg.mozilla.org/integration/autoland/rev/5078f642465ee9fb0566b22583704f6edeeb4fc5
|
||
Comment 10•3 months ago
|
||
Pushed by jmuizelaar@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fedb6cd9dddf Disable spectreJitToCxxCalls everywhere. r=jandem
|
|
||
Comment 11•3 months ago
|
||
| bugherder | ||
|
||
Comment 12•3 months ago
|
||
Comment on attachment 9377326 [details]
Bug 1877605. Disable spectreJitToCxxCalls everywhere.
Approved for 123 beta 6, thanks.
|
|
||
Comment 13•3 months ago
|
||
| uplift | ||
https://hg.mozilla.org/releases/mozilla-beta/rev/6b35a60782fb
|
|
||
Comment 14•3 months ago
|
||
| bugherder uplift | ||
|
||
Comment 15•3 months ago
|
||
(In reply to Pulsebot from comment #6)
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dabc78ed3644
Disable spectreJitToCxxCalls everywhere. r=jandem
== Change summary for alert #41256 (as of Wed, 31 Jan 2024 22:48:17 GMT) ==
Improvements:
| Ratio | Test | Platform | Options | Absolute values (old vs new) | Performance Profiles |
|---|---|---|---|---|---|
| 3% | speedometer3 | macosx1300-64-shippable-qr | fission webrender | 31.23 -> 32.02 | Before/After |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=41256
|
||
Comment 16•3 months ago
|
||
Most other benchmarks improved too:
1.3% on AWFY-Ares6
0.2% on AWFY-Jetstream2
0.5%-1% on AWFY-SP2
1% on AWFY-Sunspider
A lot of improvements on DevTools tests
== Change summary for alert #41293 (as of Fri, 02 Feb 2024 15:12:47 GMT) ==
Improvements:
| Ratio | Test | Platform | Options | Absolute values (old vs new) |
|---|---|---|---|---|
| 15% | damp source-map.SourceMapGenerator-toString.DAMP | macosx1015-64-shippable-qr | e10s fission stylo webrender | 339.63 -> 287.67 |
| 15% | damp source-map.SourceMapGenerator-toString.DAMP | macosx1015-64-shippable-qr | e10s fission stylo webrender-sw | 344.84 -> 293.09 |
| 14% | damp source-map.SourceMapGenerator-toString.DAMP | windows10-64-shippable-qr | e10s fission stylo webrender | 328.80 -> 281.43 |
| 14% | damp source-map.SourceMapGenerator-toString.DAMP | windows10-64-shippable-qr | e10s fission stylo webrender-sw | 329.68 -> 282.55 |
| 8% | damp source-map.originalPositionFor.DAMP | windows10-64-shippable-qr | e10s fission stylo webrender-sw | 268.44 -> 247.05 |
| ... | ... | ... | ... | ... |
| 2% | damp console.log-in-loop-content-process-error | windows10-64-shippable-qr | e10s fission stylo webrender | 91.71 -> 89.85 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=41293
|
||
Updated•3 months ago
|
|
||
Updated•3 months ago
|
|
||
Comment 18•3 months ago
|
||
== Change summary for alert #41295 (as of Fri, 02 Feb 2024 21:19:25 GMT) ==
Improvements:
| Ratio | Test | Platform | Options | Absolute values (old vs new) |
|---|---|---|---|---|
| 10% | dromaeo_css | windows10-64-shippable-qr | e10s fission stylo webrender | 23,137.72 -> 25,355.40 |
| 9% | dromaeo_css | macosx1015-64-shippable-qr | e10s fission stylo webrender | 19,824.69 -> 21,621.37 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=41295
Description
•