1711128 - Revert just the GCReason changes of bug 1692308

Status: RESOLVED FIXED

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

Description

[Steve Fink [:sfink] [:s:]]

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.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1711128 - Slightly simplify code for waiting for parent to let us GC

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1711128 - Rename GCRunnerActions: <MajorGC,MajorGCReady> -> <WaitToMajorGC, StartMajorGC>

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1711128 - Eliminate (rare) spurious GCs if we start and finish a new GC while waiting for a response from the parent. Before this patch, when the response arrived from the parent we would unconditionally create a GCRunner and use the GCReason...

...remembered from when we made the request to the parent.

Comment 4

[Paul Bone [:pbone]]

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

[Paul Bone [:pbone]]

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?

Comment 6

[Steve Fink [:sfink] [:s:]]

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

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1711128 - Revert to pre-1692308 GCReason handling

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 8

[Pulsebot]

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

Comment 9

[Iulian Moraru]

Backed out for causing bc failures on browser_gc_schedule.js.

Push with failures

Failure log

Backout link

Comment 10

[Pulsebot]

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

Comment 11

[Narcis Beleuzu [:NarcisB]]

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