1668831 - Warp: Transpile CacheIR instructions for DOM expandos

Status: RESOLVED FIXED

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

Description

[Tom S [:evilpie]]

For something like document.getElementById("abc"), the getElementById lookup is a "DOMProxyUnshadowed" stub.
For that we should transpile LoadDOMExpandoValueGuardGeneration, LoadDOMExpandoValue and GuardDOMExpandoMissingOrGuardShape.

This would actually give us an edge over Ion, it always uses an IC for this. And we can use every DOM performance improvement we can get.

Comment 1

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99f7db259163d6190de6937b224d3919723851a4

Comment 2

[Tom S [:evilpie]]

Let's extend this to cover transpiling all instructions related to DOM expandos. I think we need to be able to transpile all these instructions for the alias analysis to understand DOM calls better. Otherwise for stuff like getElementById we will always have some MGetPropertyCache instruction in between. I am actually not quite sure how TI optimizes this. I think it basically bakes in the actual function pointer and thus it doesn't directly depend on the load.

In this dromaeo "getElementById" benchmark almost all instructions are hoisted outside the loop, but there is still an IC for the lookup. The actual function call is hoisted.

Comment 3

[Tom S [:evilpie]]

Attachment: Bug 1668831 - Transpile CacheIR instructions for DOM expandos. r?jandem

Comment 4

[Jan de Mooij [:jandem]]

Unfortunately we have code to update the generation stub field. I'm a bit worried about bailouts from that.

Can you see if updating the generation is common? Maybe we should stop doing that or maybe we can set a flag on the CacheIR stub when that happens and then we skip transpiling the stub...

Comment 5

[Tom S [:evilpie]]

Attachment: Bug 1668831 - Add an alias set for DOM proxy expandos. r?jandem

The DOM proxy expando MIR instructions load from the DOM privateSlot and ExpandoAndGeneration.
As far as I can tell this is basically distinct from any other currently existing alias set.
AliasSet::FixedSlot is maybe the closest, but the the DOM privateSlot is actually not a fixed slot in the typical sense.

We need to /something/ that doesn't include FixedSlot however to sucessfully optimize Dromaeo benchmark, which has a
MStoreFixedSlot instruction in the loop, which would prevent any of the hoisting we are hoping for.

AliasSet::DOMProxyExpando is similar to the existing AliasSet::DOMProperty in the sense that both are only used as a AliasSet::Load,
and implicitly by AliasSet::Any.

Depends on D92328

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c32ff66e812a
Transpile CacheIR instructions for DOM expandos. r=jandem
https://hg.mozilla.org/integration/autoland/rev/369c8fff89d2
Add an alias set for DOM proxy expandos. r=jandem

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c32ff66e812a
https://hg.mozilla.org/mozilla-central/rev/369c8fff89d2

Comment 8

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #27212 (as of Wed, 14 Oct 2020 01:28:09 GMT) ==

Improvements:

26% dromaeo_dom linux64-shippable-qr opt e10s stylo 2,733.25 -> 3,434.22
23% dromaeo_dom linux64-shippable opt e10s stylo 2,774.31 -> 3,425.81

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=27212