Closed Bug 1711128 Opened 3 years ago Closed 3 years ago

Revert just the GCReason changes of bug 1692308

Categories

(Core :: JavaScript: GC, task, P3)

task

Tracking

()

RESOLVED FIXED
90 Branch
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: nobody → sphink
Status: NEW → ASSIGNED
Blocks: 1711135

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?

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?

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

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.

Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f555b85f8338 Slightly simplify code for waiting for parent to let us GC r=pbone,smaug https://hg.mozilla.org/integration/autoland/rev/64242f7debd4 Rename GCRunnerActions: <MajorGC,MajorGCReady> -> <WaitToMajorGC, StartMajorGC> r=pbone,smaug https://hg.mozilla.org/integration/autoland/rev/a686a2053da4 Revert to pre-1692308 GCReason handling r=pbone,smaug https://hg.mozilla.org/integration/autoland/rev/efb23924bb9c Eliminate (rare) spurious GCs if we start and finish a new GC while waiting r=pbone,smaug

Backed out for causing bc failures on browser_gc_schedule.js.

Push with failures

Failure log

Backout link

Flags: needinfo?(sphink)
Blocks: 1703443
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8c14d707fd1a Slightly simplify code for waiting for parent to let us GC r=pbone,smaug https://hg.mozilla.org/integration/autoland/rev/9b61f8b3962d Rename GCRunnerActions: <MajorGC,MajorGCReady> -> <WaitToMajorGC, StartMajorGC> r=pbone,smaug https://hg.mozilla.org/integration/autoland/rev/799b2049bfd1 Revert to pre-1692308 GCReason handling r=pbone,smaug https://hg.mozilla.org/integration/autoland/rev/4593420ce2c8 Eliminate (rare) spurious GCs if we start and finish a new GC while waiting r=pbone,smaug
Flags: needinfo?(sphink)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: