1799023 - Use generation counter instead of shape guard for GetGName

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Iain Ireland [:iain]]

Currently, when accessing a global variable, we generate CacheIR similar to this:

      GuardShape                    objId 0, shapeOffset 0
      LoadEnclosingEnvironment      objId 0, resultId 1
      GuardShape                    objId 1, shapeOffset 8
      LoadDynamicSlotResult         objId 1, offsetOffset 16
      ReturnFromIC

This guards the shape of the global lexical object and the global object. If those shapes change (for example, because a new global variable has been declared) then the shapeguards fail and we need to attach a new IC. If this happens often enough, we go megamorphic and eventually fall back to a generic call.

In most cases, a shape guard is too aggressive here. If we add a new property, existing properties don't change slot. If we have a dedicated generational counter (probably living on the realm) that is only bumped when something meaningful changes (like the deletion of a property from the global) then we can avoid churn and generate faster code.

Profiles of this spreadsheet show a large number of GetName fallback calls, so it may benefit from doing something along these lines.

Comment 1

[Iain Ireland [:iain]]

Attachment: Bug 1799023: Add generation counter to global object r=jandem

This patch adds a counter to each global object that can be guarded on to validate that previous property lookups are still valid. It is bumped if a new global lexical variable shadows an existing property of the global object, if a property of the global object is deleted, or if a property of the global object is changed from a data property to an accessor (or vice versa). Notably, it is not bumped when a new property is defined on the global object, which eliminates a large number of spurious failing guards.

Comment 2

[Iain Ireland [:iain]]

Attachment: Bug 1799023: Use global counter in CacheIR for GetGName r=jandem

Depends on D170959

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1799023: Transpile GuardGlobalGeneration r=jandem

Depends on D170960

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1799023: Add testcases r=jandem

Depends on D170961

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1799023: Add AssertPropertyLookup r=jandem!

To avoid slowing debug builds down too much, I'm currently only turning this on in baseline.

Depends on D170962

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/673700951b68
Add generation counter to global object r=jandem
https://hg.mozilla.org/integration/autoland/rev/aca07fd972df
Use global counter in CacheIR for GetGName r=jandem
https://hg.mozilla.org/integration/autoland/rev/ae0512a7262c
Transpile GuardGlobalGeneration r=jandem
https://hg.mozilla.org/integration/autoland/rev/0565f6b81c8d
Add testcases r=jandem
https://hg.mozilla.org/integration/autoland/rev/56f65b56a14d
Add AssertPropertyLookup r=jandem

Comment 7

[Norisz Fay [:noriszfay]]

Backed out for causing bustages on VMFunctions.cpp

Backout link

Push with failures

Failure log

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/996f1a539167
Add generation counter to global object r=jandem
https://hg.mozilla.org/integration/autoland/rev/8732055efae2
Use global counter in CacheIR for GetGName r=jandem
https://hg.mozilla.org/integration/autoland/rev/0475dade1265
Transpile GuardGlobalGeneration r=jandem
https://hg.mozilla.org/integration/autoland/rev/cbcfc6dcb563
Add testcases r=jandem
https://hg.mozilla.org/integration/autoland/rev/8b9e2deb1253
Add AssertPropertyLookup r=jandem

Comment 9

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/996f1a539167
https://hg.mozilla.org/mozilla-central/rev/8732055efae2
https://hg.mozilla.org/mozilla-central/rev/0475dade1265
https://hg.mozilla.org/mozilla-central/rev/cbcfc6dcb563
https://hg.mozilla.org/mozilla-central/rev/8b9e2deb1253

Comment 10

[Mayank Bansal]

Lots of improvements on AWFY Speedometer and Jetstream

Comment 11

[Alex Finder-PTO|back on Sept 16th]

(In reply to Cristian Tuns from comment #9)

https://hg.mozilla.org/mozilla-central/rev/996f1a539167
https://hg.mozilla.org/mozilla-central/rev/8732055efae2
https://hg.mozilla.org/mozilla-central/rev/0475dade1265
https://hg.mozilla.org/mozilla-central/rev/cbcfc6dcb563
https://hg.mozilla.org/mozilla-central/rev/8b9e2deb1253

== Change summary for alert #37548 (as of Mon, 06 Mar 2023 14:50:25 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
22%	google-mail LastVisualChange (geomean)	macosx1015-64-shippable-qr	fission warm webrender	1,795.01 -> 1,391.56	Before/After
14%	google-docs LastVisualChange	linux1804-64-shippable-qr	fission warm webrender	6,936.58 -> 5,963.33	Before/After
14%	google-docs LastVisualChange (geomean)	linux1804-64-shippable-qr	fission warm webrender	6,914.79 -> 5,962.39	Before/After
14%	google-docs LastVisualChange (mean)	linux1804-64-shippable-qr	fission warm webrender	6,921.41 -> 5,977.56	Before/After
13%	google-docs LastVisualChange	windows10-64-shippable-qr	cold fission webrender	5,563.67 -> 4,861.08	Before/After
...	...	...	...	...	...
2%	linkedin loadtime (geomean)	windows10-64-shippable-qr	cold fission webrender	2,429.92 -> 2,374.73

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=37548