Closed Bug 1799023 Opened 7 months ago Closed 3 months ago

Use generation counter instead of shape guard for GetGName

Categories

(Core :: JavaScript Engine, task, P2)

task

Tracking

()

RESOLVED FIXED
112 Branch
Tracking Status
firefox112 --- fixed

People

(Reporter: iain, Assigned: iain)

References

(Blocks 3 open bugs)

Details

(Keywords: perf-alert, Whiteboard: [sp3])

Attachments

(5 files)

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.

Blocks: 1801189

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.

Depends on D170960

Depends on D170961

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

Depends on D170962

Attachment #9320911 - Attachment description: Bug 1799023: Add AssertPropertyLookup r=jandem → Bug 1799023: Add AssertPropertyLookup r=jandem!
Pushed by iireland@mozilla.com:
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

Backed out for causing bustages on VMFunctions.cpp

Backout link

Push with failures

Failure log

Flags: needinfo?(iireland)
Flags: needinfo?(iireland)
Pushed by iireland@mozilla.com:
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

Lots of improvements on AWFY Speedometer and Jetstream

Whiteboard: [sp3]

(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

Keywords: perf-alert
Blocks: 1818466
You need to log in before you can comment on or make changes to this bug.