Closed Bug 1837620 Opened 1 year ago Closed 11 months ago

Investigate removing baseline JIT guards for otherwise-dead GC things

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
117 Branch
Tracking Status
firefox117 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(Whiteboard: [sp3])

Attachments

(9 files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

This is something that has been talked about for a while. It would make some edges from JIT code weak, and sweep them when their referent died. This would also need to update JIT data structures appropriately, which may be complicated.

Without this we may be keeping garbage alive longer than necessary, and this may be affecting Speedometer3 (see bug 1821293 comment 0) although the evidence is mixed. Regardless, this would be a good improvement to make.

There is a risk that this would increase GC sweeping time but I don't have a feel for whether this will be significant on not.

Whiteboard: [sp3]

Jan can you take a look at the patch when you get a moment and let me know what you think?

Flags: needinfo?(jdemooij)

(In reply to Jon Coppeard (:jonco) from comment #2)

Jan can you take a look at the patch when you get a moment and let me know what you think?

(I commented on the patch. Approach makes sense to me, thanks for looking into this. Probably worth getting some data to see how it behaves in practice.)

Flags: needinfo?(jdemooij)
Attachment #9338923 - Attachment description: WIP: Bug 1837620 - WIP: Remove ICs that guard shapes when the shape becomes unreachable → Bug 1837620 - Part 1: Remove baseline ICs that guard shapes when the shape becomes unreachable r?jandem

Along the same lines as the previous patch, sweep object pointers stored in baseline ICs.

Depends on D180857

We skip the extra discard if we run all the way through to sweeping without
yielding, but if we start a slice in the prepare phase we will do an
unnecessary extra discard. This is probablly quite common since we always yield
at the start of an incremental GC, and budget can be long if we run in idle
time.

Using the flag rather than checking initialState makes it clearer what's going on.

Depends on D181982

This is a very minor change to cancel off-thread compilation as soon as
possible rather than leaving it running while we set up a bunch of other
sweeping work.

Depends on D181983

This is an optimisation so we don't repeatedly loop over all scripts in a zone
as there can be very many of these (e.g. 50000 is not unusual to see while
browsing around).

This adds the option to sweep JIT scripts as part of the discard options.

Depends on D181984

Updating title to reflect what these patches do.

What they don't do is to remove edges from Ion code to dead GC things. They also don't handle the multiple shape guard IC. These are opportunities for future improvement in this area.

Summary: Investigate removing JIT guards for otherwise-dead GC things → Investigate removing baseline JIT guards for otherwise-dead GC things

To make the edges for the multiple shape guard weak we need a way to find and
update the objects used to hold these edges. The simplest way to do this is to
add a vector in the zone. I don't believe that there are not huge numbers of
these objects.

Currently we iterate all BaseScripts to find the JitScripts in a zone. This is
inefficient since there can be many times more BaseScripts that JitScripts
(e.g. 10 - 20 times more).

Instead, keep all JitScripts in a zone in a linked list and use that to iterate
through them. This required giving JitScript a pointer to its owning JSScript
since that was needed in a few cases.

Attachment #9341305 - Attachment description: WIP: Bug 1837620 - Part 7: Keep JitScripts in a linked list to avoid iterating all BaseScripts → Bug 1837620 - Part 7: Keep JitScripts in a linked list to avoid iterating all BaseScripts r?jandem
Attachment #9340988 - Attachment description: Bug 1837620 - Part 6: Make edges for multiple shape guard weak too r|jandem r=sfink! → Bug 1837620 - Part 6: Make edges for multiple shape guard weak too r?jandem r=sfink!
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3853623fa933
Part 0: Removed unused argument from AutoStubFrame::enter r=jandem
https://hg.mozilla.org/integration/autoland/rev/003380bb2438
Part 1: Remove baseline ICs that guard shapes when the shape becomes unreachable r=jandem
https://hg.mozilla.org/integration/autoland/rev/5de6d8445a93
Part 2: Remove baseline ICs that guard objects when the objects becomes unreachable r=jandem
https://hg.mozilla.org/integration/autoland/rev/652e56a9fa15
Part 3: Don't discard JIT code twice if we sweep in slice started in prepare phase r=sfink
https://hg.mozilla.org/integration/autoland/rev/344fb88a0ec5
Part 4: Cancel off-thread compilation as soon as possible when sweeping r=sfink
https://hg.mozilla.org/integration/autoland/rev/318af9553464
Part 5: Sweep JIT code as part of discard when possible r=sfink
https://hg.mozilla.org/integration/autoland/rev/b6d68da027a2
Part 6: Make edges for multiple shape guard weak too r=sfink
https://hg.mozilla.org/integration/autoland/rev/4c38dd8fa0ab
Part 7: Keep JitScripts in a linked list to avoid iterating all BaseScripts r=jandem
Regressions: 1841652
Backout by abutkovits@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70723ef5e495
Backed out 8 changesets for causing high frequency failures complaining about Maybe.h. CLOSED TREE

The previous patch can remove stubs and since we don't know whether an
individual stub is a folded stub the flag can remain set after all folded
stubs are removed. This patch renames the flag to make this possibility
clearer.

Depends on D180857

Backed out for causing multiple failures.


  • Push with failures - dt
  • Failure Log
  • Failure line: PROCESS-CRASH | MOZ_ASSERT(!tc->isMarkedGray()) [@ js::gc::detail::AssertCellIsNotGray] | devtools/client/framework/browser-toolbox/test/browser_browser_toolbox_fission_inspector.js

  • Push with failures - c
  • Failure Log
  • Failure line: PROCESS-CRASH | application crashed [@ ExecutionObservableRealms::shouldRecompileOrInvalidate] | devtools/shared/webconsole/test/chrome/test_jsterm_autocomplete.html

Also these dt failures

Pushed by sstanca@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/e8fff2c187d7
Part 0: Removed unused argument from AutoStubFrame::enter r=jandem
https://hg.mozilla.org/mozilla-central/rev/393e47665e0d
Part 1: Remove baseline ICs that guard shapes when the shape becomes unreachable r=jandem
https://hg.mozilla.org/mozilla-central/rev/0f014af493fa
Part 1.5: Rename IC flag to 'mayHaveFoldedStub' to make it clear that it is no longer exact r=jandem
https://hg.mozilla.org/mozilla-central/rev/3f118d541c76
Part 2: Remove baseline ICs that guard objects when the objects becomes unreachable r=jandem
https://hg.mozilla.org/mozilla-central/rev/c2cc28741660
Part 3: Don't discard JIT code twice if we sweep in slice started in prepare phase r=sfink
https://hg.mozilla.org/mozilla-central/rev/cf221511d7b6
Part 4: Cancel off-thread compilation as soon as possible when sweeping r=sfink
https://hg.mozilla.org/mozilla-central/rev/24e0b10be67a
Part 5: Sweep JIT code as part of discard when possible r=sfink
https://hg.mozilla.org/mozilla-central/rev/bcd63afc2d34
Part 6: Make edges for multiple shape guard weak too r=sfink
https://hg.mozilla.org/mozilla-central/rev/0809497db3d0
Part 7: Keep JitScripts in a linked list to avoid iterating all BaseScripts r=jandem
https://hg.mozilla.org/mozilla-central/rev/c744a2111c3b
apply code formatting via Lando
Flags: needinfo?(jcoppeard)

For anyone trying to make sense of this, the push in comment 17 was before the backout in comment 16.

Why this is bouncing: I forgot that there's more to iterating scripts than just handing out the scripts themselves. Zone has two iterators for cells (cellIter and cellIterUnsafe) and the former does two extra things that the latter does not: it filters out dead scripts during incremental sweeping and it calls ExposeToActiveJS on the cells returned. Both of these turn out to be necessary for the debugger use of script iteration.

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3343587b5848
Part 0: Removed unused argument from AutoStubFrame::enter r=jandem
https://hg.mozilla.org/integration/autoland/rev/c444e4828105
Part 1: Remove baseline ICs that guard shapes when the shape becomes unreachable r=jandem
https://hg.mozilla.org/integration/autoland/rev/1a7f294165fb
Part 1.5: Rename IC flag to 'mayHaveFoldedStub' to make it clear that it is no longer exact r=jandem
https://hg.mozilla.org/integration/autoland/rev/14eb54dfd045
Part 2: Remove baseline ICs that guard objects when the objects becomes unreachable r=jandem
https://hg.mozilla.org/integration/autoland/rev/0beee0368a67
Part 3: Don't discard JIT code twice if we sweep in slice started in prepare phase r=sfink
https://hg.mozilla.org/integration/autoland/rev/2e6dbb731bf4
Part 4: Cancel off-thread compilation as soon as possible when sweeping r=sfink
https://hg.mozilla.org/integration/autoland/rev/ba12c0cbb7a4
Part 5: Sweep JIT code as part of discard when possible r=sfink
https://hg.mozilla.org/integration/autoland/rev/9882c0d924b8
Part 6: Make edges for multiple shape guard weak too r=sfink
https://hg.mozilla.org/integration/autoland/rev/325f1a934370
Part 7: Keep JitScripts in a linked list to avoid iterating all BaseScripts r=jandem
https://hg.mozilla.org/integration/autoland/rev/74c699614101
apply code formatting via Lando
Regressions: 1842364
Regressions: 1842584
Regressions: 1842617
Regressions: 1841650
Regressions: 1845248
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: