1837620 - Investigate removing baseline JIT guards for otherwise-dead GC things

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

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.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1837620 - Part 1: Remove baseline ICs that guard shapes when the shape becomes unreachable r?jandem

Comment 2

[Jon Coppeard (:jonco)]

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

Comment 3

[Jan de Mooij [:jandem]]

(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.)

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1837620 - Part 0: Removed unused argument from AutoStubFrame::enter r?jandem

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1837620 - Part 2: Remove baseline ICs that guard objects when the objects becomes unreachable r?jandem

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

Depends on D180857

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1837620 - Part 3: Don't discard JIT code twice if we sweep in slice started in prepare phase r?sfink

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

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1837620 - Part 4: Cancel off-thread compilation as soon as possible when sweeping r?sfink

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

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1837620 - Part 5: Sweep JIT code as part of discard when possible r?sfink

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

Comment 9

[Jon Coppeard (:jonco)]

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.

Comment 10

[Jon Coppeard (:jonco)]

Attachment: Bug 1837620 - Part 6: Make edges for multiple shape guard weak too r?jandem r=sfink!

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.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: Bug 1837620 - Part 7: Keep JitScripts in a linked list to avoid iterating all BaseScripts r?jandem

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70723ef5e495
Backed out 8 changesets for causing high frequency failures complaining about Maybe.h. CLOSED TREE

Comment 14

[Atila Butkovits]

Backed out for causing high frequency failures complaining about WarpOracle.cpp.

Backout link: https://hg.mozilla.org/integration/autoland/rev/70723ef5e495c2d58c42a340eebd16aeface5e09

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=cf717d6692eb6ff08bb1d0342645c7c6c1dcf8f7&selectedTaskRun=YbSw9AENQfudwvRO5yWloQ.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=421444827&repo=autoland&lineNumber=8140
https://treeherder.mozilla.org/logviewer?job_id=421456985&repo=autoland&lineNumber=21690

Comment 15

[Jon Coppeard (:jonco)]

Attachment: Bug 1837620 - Part 1.5: Rename IC flag to 'mayHaveFoldedStub' to make it clear that it is no longer exact r?jandem

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

Comment 16

[Serban Stanca [:SerbanS]]

Backed out for causing multiple failures.

• Backout link

---

• 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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Jon Coppeard (:jonco)]

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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/3343587b5848
https://hg.mozilla.org/mozilla-central/rev/c444e4828105
https://hg.mozilla.org/mozilla-central/rev/1a7f294165fb
https://hg.mozilla.org/mozilla-central/rev/14eb54dfd045
https://hg.mozilla.org/mozilla-central/rev/0beee0368a67
https://hg.mozilla.org/mozilla-central/rev/2e6dbb731bf4
https://hg.mozilla.org/mozilla-central/rev/ba12c0cbb7a4
https://hg.mozilla.org/mozilla-central/rev/9882c0d924b8
https://hg.mozilla.org/mozilla-central/rev/325f1a934370
https://hg.mozilla.org/mozilla-central/rev/74c699614101

Comment 21

[Mayank Bansal]

Wins on AWFY-Jetstream2:

64% on splay-Worst : https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&selected=3911481,1723219800&series=mozilla-central,3738094,1,13&series=mozilla-central,3738094,1,13&series=autoland,3911481,1,13&timerange=5184000&zoom=1688529824893,1689012763568,-10.437017358144235,165.008735043899

16% on splay-Average: https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&selected=3911478,1723219797&series=mozilla-central,3738091,1,13&series=mozilla-central,3738091,1,13&series=autoland,3911478,1,13&timerange=5184000&zoom=1688613715117,1688914327270,100.20399647318195,305.32120845998617\

3% on Uglify-js-wtb-first: https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&selected=3911510,1723219829&series=mozilla-central,3738123,1,13&series=mozilla-central,3738123,1,13&series=autoland,3911510,1,13&timerange=5184000&zoom=1688524258844,1689234432653,13.965014460461951,21.466179281036236

Regressions on AWFY-Jetstream2:

26% on Splay-First: https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&selected=3911479,1723219798&series=mozilla-central,3738092,1,13&series=mozilla-central,3738092,1,13&series=autoland,3911479,1,13&timerange=5184000&zoom=1688387107669,1688948918093,73.24412580651928,398.6379674557832