Closed Bug 1380387 Opened 5 years ago Closed 5 years ago

We shouldn't repeat GCs for roots removed between GC slices

Categories

(Core :: JavaScript: GC, enhancement)

55 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(2 files)

Following on from bug 1380025, I realised that we end up calling notifyRootsRemoved() between slices due to the use of things like XPCJSObjectHolder.  The intention was to detect only when a finalizer removed a root as this indicates something that we assumed was live at the start of the GC might have in fact been garbage.

The current situation leads to the possibility of endlessly repeating GCs since I think this class and others like it are widely used.

It's possible that we should just remove this whole mechanism.  Maybe we would get shutdown leaks though, I'm not sure.

The patch updates the code to the original intent.
Attachment #8885813 - Flags: review?(sphink)
Is this going to actually be called during the GC? Don't we do deferred finalization for these objects?
Attachment #8885813 - Flags: review?(sphink) → review+
(In reply to Andrew McCreight [:mccr8] from comment #1)
That's a good point.  If we 'poke' the GC after it has finished then it won't start another one anyway so non-incremental GCs are not affected.  It's not clear whether this mechanism was meant to work for incremental GCs.  I'll think some more about this.
Alternatively, let's just make this happen for shutdown GCs as originally intended.  I ran this through try and it didn't seem to cause any leaks.
Assignee: nobody → jcoppeard
Attachment #8886624 - Flags: review?(sphink)
(In reply to Jon Coppeard (:jonco) from comment #3)
> Alternatively, let's just make this happen for shutdown GCs as originally
> intended.  I ran this through try and it didn't seem to cause any leaks.

The thing to check on try is if you start seeing intermittent OOM crashes in win32 debug reftests.
Attachment #8886624 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15c280b7d53b
Only repeat shutdown GCs for removed roots r=sfink
Backed out bug 1380967 and bug 1380387 for Linux cgc failure in js1_8_5/extensions/collect-gray.js:

bug 1380387:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b292385f25f7621f56409342a964ce6c6973c7a4

bug 1380967:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ee8ecd9aca1f72aa0c090c20ef9bd26f20a37e7

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e029feccd0c06792ec5b623ba2c1343272c195ab&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=114435841&repo=mozilla-inbound
[task 2017-07-14T18:39:30.558405Z] ## js1_8_5/extensions/collect-gray.js: rc = 3, run time = 0.297144
[task 2017-07-14T18:39:30.558481Z] 1337209: Test gray marking
[task 2017-07-14T18:39:30.558546Z] js1_8_5/extensions/collect-gray.js:109:1 Error: Assertion failed: got "unmarked", expected "gray": black map, gray delegate => gray key
[task 2017-07-14T18:39:30.558579Z] Stack:
[task 2017-07-14T18:39:30.558616Z]   @js1_8_5/extensions/collect-gray.js:109:1
[task 2017-07-14T18:39:30.558660Z] TEST-UNEXPECTED-FAIL | js1_8_5/extensions/collect-gray.js | (args: "")
Flags: needinfo?(jcoppeard)
We need to disable GC zeal in that test otherwise it risks being broken by anything that disturbs GC timing.
Flags: needinfo?(jcoppeard)
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb92b29f5500
Only repeat shutdown GCs for removed roots r=sfink
https://hg.mozilla.org/mozilla-central/rev/eb92b29f5500
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.