Revert just the GCReason changes of bug 1692308
Categories
(Core :: JavaScript: GC, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox90 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(4 files)
See bug 1692308. It changed GCReason tracking such that Gecko-scheduled GCs started reporting INTER_SLICE_GC as their reason rather than their triggering reason. Which is kind of good, but not really intended for that patch. Also, the resulting GCReason management was still kind of weird and inconsistent.
Then bug 1629064 added some additional logic that interacted with GCReason management.
I'd like to back up the reason management to as close as I can get it to pre-bug 1692308, and then in another bug propose an alteration that I think will be both simpler and more useful.
However, bug 1629064 uses the main state variable (sSchedule.mMajorGCReason) to track some things for its own purposes, so currently it looks like I'll need to incorporate the changes from bug 1710552 as well.
Now if bug 1629064 gets backed out for random perf regressions, I'll have to go about this differently.
Assignee | ||
Comment 1•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
Assignee | ||
Comment 3•3 years ago
|
||
...remembered from when we made the request to the parent.
Comment 4•3 years ago
|
||
hi sfink, Do I need to go about Bug 1710552 differently so our changes can merge? Do I need to wait for this to land first?
Comment 5•3 years ago
|
||
Also in Bug 1703443 I wanted to move more of the "is now a good time to GC?" logic from nsJSEnvironment.cpp
into CCGCScheduler
and keep all its state there. Does that match with where you see things ending up?
Assignee | ||
Comment 6•3 years ago
|
||
(In reply to Paul Bone [:pbone] from comment #4)
hi sfink, Do I need to go about Bug 1710552 differently so our changes can merge? Do I need to wait for this to land first?
If we go with the patches here, then they will also resolve bug 1710552. I did not dupe that bug to this yet, because I wasn't sure if my approach would be accepted.
Assignee | ||
Comment 7•3 years ago
|
||
Bug 1692308 made some changes to how GCReasons are reported.
Previously, the reasons reported for slices depended on how the GC was
triggered: internally triggered GCs would have the internal reason for their
first slice and INTER_SLICE_GC for every later slice. PokeGC-triggered GCs
would use the PokeGC reason for all slices. (Or if RunNextCollectorTimer was
called before the GC began, its reason would get swapped in for the PokeGC
one.)
After bug 1692308, INTER_SLICE_GC was used for all non-initial slices. This
change wasn't entirely intentional (part of it was).
This patch reverts to the previous behavior, and in so doing adjusts the use of
sScheduler.mMajorGCReason (it maintains the reason that we want to use for
later slices, rather than clearing it when we get the go-ahead from the
parent.)
Note that this handling of GCReasons is intended to be temporary. I want to fix
it properly in a later bug.
Comment 9•3 years ago
|
||
Backed out for causing bc failures on browser_gc_schedule.js.
Comment 10•3 years ago
|
||
Comment 11•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8c14d707fd1a
https://hg.mozilla.org/mozilla-central/rev/9b61f8b3962d
https://hg.mozilla.org/mozilla-central/rev/799b2049bfd1
https://hg.mozilla.org/mozilla-central/rev/4593420ce2c8
Assignee | ||
Updated•3 years ago
|
Description
•