Remove uses of arguments rectifier in EnterJit
Categories
(Core :: JavaScript Engine: JIT, task, P1)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox145 | --- | fixed |
People
(Reporter: iain, Assigned: iain)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
Instead of supporting argument underflow in the EnterJit trampolines, we pass the arguments rectifier as the code pointer. We can do better.
|
Assignee | |
Comment 1•8 months ago
|
||
To avoid having to rewrite ~7 arch-specific EnterJit stubs to remove the need for a rectifier frame, this patch adds a shared path for all architectures (except arm64; see next patch).
I initially tried to rewrite the calling convention to eliminate the awkward way that actualArgs is passed in the result value, under the assumption that it was always the case that actualArgs+1 == maxArgc. However, it turns out that's not true when doing OSR into blinterp. In that case, we already padded the arguments array with undefined when doing the initial call, so argc (the size of the argv array) is max(actuals, formals). However, when creating the frame descriptor, we need to use the original actualArgs value, because it's observable for things like arguments.length. So I rewrote the patch to reverse the removal of actualArgs. I left in my changes to pass argc/argv without this, because it simplified codegen. I added a testcase that would have caught my initial bug.
The only part of this trampoline that needs to be architecture-specific is the ABI calling convention. We could (and probably should) move more code into generateEnterJitShared, but this was a good dividing line for now that lets us eliminate the rectifier stub. I opened bug 1989118 to track the remaining work.
I didn't run the entire testsuite on the unsupported platforms (mips/loong/riscv), but I did verify that each platform compiles and can run basic tests.
|
|
Assignee | |
Comment 2•8 months ago
|
||
On arm64, the presence of the pseudo-stack pointer means that we would really prefer to allocate the stack frame first and fill it in, instead of doing a bunch of pushes. Instead of using the shared code, which does a ton of pushes, I rewrote an arm64-specific version of the equivalent code that doesn't have to wrestle with the PSP.
Note that in this case, we have enough registers available to keep the number of actual arguments live in a register between the underflow check and the point where we push the descriptor, so we don't have to read it out of the return value. I couldn't quite make that work in the shared code.
|
||
Comment 5•8 months ago
|
||
Backed out for causing build bustages
- Backout link
- Push with failures
- Failure Log
- Failure line: /builds/worker/checkouts/gecko/js/src/jit/x64/Trampoline-x64.cpp(145,8): error: no matching member function for call to 'mov'
|
||
Comment 7•8 months ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/2a6ac4c80c45
https://hg.mozilla.org/mozilla-central/rev/e3fff5ed6dd1
https://hg.mozilla.org/mozilla-central/rev/daab717b33f8
|
|
Assignee | |
Comment 8•7 months ago
|
||
|
|
Assignee | |
Comment 9•7 months ago
|
||
|
||
Comment 10•7 months ago
|
||
A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)
|
||
Comment 11•7 months ago
|
||
|
||
Comment 12•7 months ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 13•7 months ago
|
||
Oops, the second set of patches were supposed to be attached to bug 1992001.
|
||
Updated•7 months ago
|
|
||
Comment 15•7 months ago
|
||
(In reply to Pulsebot from comment #11)
Pushed by iireland@mozilla.com:
https://github.com/mozilla-firefox/firefox/commit/3242115cd4ed
https://hg.mozilla.org/integration/autoland/rev/e3d38c1bf4b2
Don't build rectifier frames during bailouts r=jandem
https://github.com/mozilla-firefox/firefox/commit/6f9981e7e106
https://hg.mozilla.org/integration/autoland/rev/53028076ae8a
Remove dead rectifier code r=jandem
https://github.com/mozilla-firefox/firefox/commit/e5054797989f
https://hg.mozilla.org/integration/autoland/rev/2565d1593400
1991223: apply code formatting via Lando
Hello!
Do you think it's possible the push mentioned in the referenced comment could have caused the improvement below ?
We are unable to run backfills on the following push range, and it's difficult to determine which of the patches in that push range caused the improvement.
Perfherder has detected a browsertime performance change from push e3f1fd706322ba441f886715b7d6dc5cf1e62c60.
If you have any questions, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.
Improvements:
| Ratio | Test | Platform | Options | Absolute values (old vs new) |
|---|---|---|---|---|
| 5% | speedometer React-TodoMVC/DeletingAllItems | windows11-64-24h2-shippable | fission webrender | 11.45 -> 10.92 |
Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.
If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.
You can run all of these tests on try with ./mach try perf --alert 47215
The following documentation link provides more information about this command.
|
|
Assignee | |
Comment 16•7 months ago
|
||
It's possible that this helped, but if I had to bet on a patch in that push range, I would go with bug 1991101. I don't recall seeing many samples in the rectifier trampoline prior to this change.
Description
•