1703469 - Optimize calls to polymorphic scripted getters/setters better

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P3)

Description

[Jan de Mooij [:jandem]]

After bug 1700052, getter/setter IC stubs still optimize for a single getter/setter object, but we could now change that to load the getter or setter object from the GetterSetter and guard on its BaseScript (or handle any scripted function), similar to normal calls.

We probably don't want to do this always because this guard would be a bit slower than what we do now, but maybe when we're attaching a stub other than the first one we could use the slower guard that handles more cases (similar to normal calls).

It might also make sense to have a shape attribute indicating whether the accessors are scripted functions, so that we don't need to guard on the JSClass in the common case.

Comment 2

[Iain Ireland [:iain]]

Attachment: Bug 1703469: Use operand for callee in scripted getters r=jandem

This should generate roughly the same code as before. The one exception is in the Ion CacheIR compiler, where we no longer have direct access to the callee, so if it's a cross-realm getter, we have to load the correct realm out of the callee using switchToObjectRealm (instead of baking it in with switchToRealm). It didn't seem worth adding an entire extra operand to make that particular case a bit faster. (The same comment applies to the next patch, which does the same as this one, but for setters.)

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1703469: Use operand for callee in scripted setters r=jandem

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1703469: Move getter/setter helpers inside IRGenerator r=jandem

This patch only moves code around, and should have no semantic changes.

Moving EmitGuardGetterSetterSlot inside the IR generator lets us access isFirstStub_ in the same way as emitCalleeGuard, which will be used in the next patch.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1703469: Guard on script instead of function for non-first getter/setters r=jandem

Caching the callee inside the CacheIRWriter seemed simpler than trying to rewrite all the callers of emitGuardGetterSetterSlot / emitCallGetterResultGuards / etc to conditionally return an operand for the callee (only when scripted).

LoadGetterSetterFunction unfortunately has to guard that the result is a function, because it is possible to create a GetterSetter containing other types. We do enforce that the accessor function must be callable, but that allows bound functions and callable proxies. We could consider adding a shape flag for objects that have a non-JSFunction getter/setter, or alternatively low-bit tagging the setter if the GetterSetter contains a non-JSFunction, to avoid the class guard. Removing the class guard is about a 15-25% improvement on the microbenchmark in bug 1962778. We would get all that improvement with a shape flag, or most of that improvement with a low-bit tag. We could also consider a fuse.

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1703469: Remove redundant function arg r=jandem

We're always using the canonical function here anyway.

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1703469: Use JSScript instead of JSFunction in trial inlining r=jandem

We inline scripts, not functions, so switching to scripts as soon as possible in the inliner makes everything a little cleaner.

Comment 8

[Iain Ireland [:iain]]

Attachment: Bug 1703469: Inline script-guarded getters and setters r=jandem

Comment 9

[Iain Ireland [:iain]]

Attachment: Bug 1703469: Fix PBL r=jandem

Comment 10

[Iain Ireland [:iain]]

Attachment: Bug 1703469: Add more robust testcase r=jandem

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f4f5b7d47d29
https://hg.mozilla.org/integration/autoland/rev/9ab7a2a9368b
Use operand for callee in scripted getters r=jandem
https://github.com/mozilla-firefox/firefox/commit/f55e9e66da59
https://hg.mozilla.org/integration/autoland/rev/5d869371832c
Use operand for callee in scripted setters r=jandem
https://github.com/mozilla-firefox/firefox/commit/237d5105a7b3
https://hg.mozilla.org/integration/autoland/rev/b5f9e08eacd4
Move getter/setter helpers inside IRGenerator r=jandem
https://github.com/mozilla-firefox/firefox/commit/b1220f6fea23
https://hg.mozilla.org/integration/autoland/rev/c7177dd76450
Guard on script instead of function for non-first getter/setters r=jandem
https://github.com/mozilla-firefox/firefox/commit/aaa390390b56
https://hg.mozilla.org/integration/autoland/rev/99a53fc8491c
Remove redundant function arg r=jandem
https://github.com/mozilla-firefox/firefox/commit/33cd9a6366fc
https://hg.mozilla.org/integration/autoland/rev/74846c518595
Use JSScript instead of JSFunction in trial inlining r=jandem
https://github.com/mozilla-firefox/firefox/commit/63b530b2724a
https://hg.mozilla.org/integration/autoland/rev/6a5e7d6eb97d
Inline script-guarded getters and setters r=jandem
https://github.com/mozilla-firefox/firefox/commit/0ed90c8ce56c
https://hg.mozilla.org/integration/autoland/rev/76a206c95c72
Fix PBL r=jandem
https://github.com/mozilla-firefox/firefox/commit/7cc77752f8af
https://hg.mozilla.org/integration/autoland/rev/8f4c08f411bd
Add more robust testcase r=jandem
https://github.com/mozilla-firefox/firefox/commit/cd8338935daf
https://hg.mozilla.org/integration/autoland/rev/9b1cfca364eb
apply code formatting via Lando

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2e62edcffb55
https://hg.mozilla.org/integration/autoland/rev/b8e4efef7a38
Revert "Bug 1703469: apply code formatting via Lando" for causing crashes @ js::jit::MGuardFunctionScript::getAliasSet

Comment 13

[amarc]

Backed out for causing crashes @ js::jit::MGuardFunctionScript::getAliasSet

• Backout link
• Push with failures
• Failure Log

Comment 14

[Iain Ireland [:iain]]

Attachment: Bug 1703469: Add FunctionHasStableBaseScript r=jandem

Some Marionette tests failed on the assertion here because we had a getter calling a non-lambda self-hosted function. The fix is to reuse the checks we already have in emitCalleeGuard.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8fd1f90f0329
https://hg.mozilla.org/integration/autoland/rev/c2c18f47bd75
Use operand for callee in scripted getters r=jandem
https://github.com/mozilla-firefox/firefox/commit/15e08133d7ff
https://hg.mozilla.org/integration/autoland/rev/4f52d83bba11
Use operand for callee in scripted setters r=jandem
https://github.com/mozilla-firefox/firefox/commit/82fc1f68399a
https://hg.mozilla.org/integration/autoland/rev/8a6a0e4222ec
Move getter/setter helpers inside IRGenerator r=jandem
https://github.com/mozilla-firefox/firefox/commit/336377011274
https://hg.mozilla.org/integration/autoland/rev/975ee0d7b24a
Guard on script instead of function for non-first getter/setters r=jandem
https://github.com/mozilla-firefox/firefox/commit/4e6df2c1ff64
https://hg.mozilla.org/integration/autoland/rev/ce24a23a4177
Remove redundant function arg r=jandem
https://github.com/mozilla-firefox/firefox/commit/485efc11f47f
https://hg.mozilla.org/integration/autoland/rev/d8a12057d549
Use JSScript instead of JSFunction in trial inlining r=jandem
https://github.com/mozilla-firefox/firefox/commit/85fe7e829d71
https://hg.mozilla.org/integration/autoland/rev/a794c182d5a7
Inline script-guarded getters and setters r=jandem
https://github.com/mozilla-firefox/firefox/commit/26b36bd70bf2
https://hg.mozilla.org/integration/autoland/rev/d0971ea3d1e5
Fix PBL r=jandem
https://github.com/mozilla-firefox/firefox/commit/ac74992f2005
https://hg.mozilla.org/integration/autoland/rev/cad662a9d760
Add more robust testcase r=jandem
https://github.com/mozilla-firefox/firefox/commit/a0c7d51190d5
https://hg.mozilla.org/integration/autoland/rev/89b80b7afa64
Add FunctionHasStableBaseScript r=jandem
https://github.com/mozilla-firefox/firefox/commit/f968cd9ed08b
https://hg.mozilla.org/integration/autoland/rev/bafd318ef057
1970922: apply code formatting via Lando

Comment 16

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/c2c18f47bd75
https://hg.mozilla.org/mozilla-central/rev/4f52d83bba11
https://hg.mozilla.org/mozilla-central/rev/8a6a0e4222ec
https://hg.mozilla.org/mozilla-central/rev/975ee0d7b24a
https://hg.mozilla.org/mozilla-central/rev/ce24a23a4177
https://hg.mozilla.org/mozilla-central/rev/d8a12057d549
https://hg.mozilla.org/mozilla-central/rev/a794c182d5a7
https://hg.mozilla.org/mozilla-central/rev/d0971ea3d1e5
https://hg.mozilla.org/mozilla-central/rev/cad662a9d760
https://hg.mozilla.org/mozilla-central/rev/89b80b7afa64
https://hg.mozilla.org/mozilla-central/rev/bafd318ef057

Comment 17

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #15)

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8fd1f90f0329
https://hg.mozilla.org/integration/autoland/rev/c2c18f47bd75
Use operand for callee in scripted getters r=jandem
https://github.com/mozilla-firefox/firefox/commit/15e08133d7ff
https://hg.mozilla.org/integration/autoland/rev/4f52d83bba11
Use operand for callee in scripted setters r=jandem
https://github.com/mozilla-firefox/firefox/commit/82fc1f68399a
https://hg.mozilla.org/integration/autoland/rev/8a6a0e4222ec
Move getter/setter helpers inside IRGenerator r=jandem
https://github.com/mozilla-firefox/firefox/commit/336377011274
https://hg.mozilla.org/integration/autoland/rev/975ee0d7b24a
Guard on script instead of function for non-first getter/setters r=jandem
https://github.com/mozilla-firefox/firefox/commit/4e6df2c1ff64
https://hg.mozilla.org/integration/autoland/rev/ce24a23a4177
Remove redundant function arg r=jandem
https://github.com/mozilla-firefox/firefox/commit/485efc11f47f
https://hg.mozilla.org/integration/autoland/rev/d8a12057d549
Use JSScript instead of JSFunction in trial inlining r=jandem
https://github.com/mozilla-firefox/firefox/commit/85fe7e829d71
https://hg.mozilla.org/integration/autoland/rev/a794c182d5a7
Inline script-guarded getters and setters r=jandem
https://github.com/mozilla-firefox/firefox/commit/26b36bd70bf2
https://hg.mozilla.org/integration/autoland/rev/d0971ea3d1e5
Fix PBL r=jandem
https://github.com/mozilla-firefox/firefox/commit/ac74992f2005
https://hg.mozilla.org/integration/autoland/rev/cad662a9d760
Add more robust testcase r=jandem
https://github.com/mozilla-firefox/firefox/commit/a0c7d51190d5
https://hg.mozilla.org/integration/autoland/rev/89b80b7afa64
Add FunctionHasStableBaseScript r=jandem
https://github.com/mozilla-firefox/firefox/commit/f968cd9ed08b
https://hg.mozilla.org/integration/autoland/rev/bafd318ef057
1970922: apply code formatting via Lando

Perfherder has detected a browsertime performance change from push bafd318ef057aae57bb7df6b59080f8e5be2d1d7.

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)	Performance Profiles
6%	speedometer VueJS-TodoMVC/CompletingAllItems/Sync	macosx1500-aarch64-shippable	fission webrender	0.34 -> 0.32	Before/After
3%	speedometer React-Redux-TodoMVC/DeletingItems	macosx1500-aarch64-shippable	fission webrender	16.95 -> 16.43	Before/After
3%	speedometer React-Redux-TodoMVC/DeletingItems/Sync	macosx1500-aarch64-shippable	fission webrender	16.71 -> 16.24	Before/After
3%	speedometer React-Redux-TodoMVC/CompletingAllItems/Sync	macosx1500-aarch64-shippable	fission webrender	27.43 -> 26.69	Before/After
3%	speedometer React-Redux-TodoMVC/CompletingAllItems	macosx1500-aarch64-shippable	fission webrender	28.24 -> 27.51	Before/After
...	...	...	...	...	...
2%	speedometer React-Redux-TodoMVC	macosx1500-aarch64-shippable	fission webrender	65.31 -> 63.70	Before/After

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 45617

The following documentation link provides more information about this command.