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

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

Attachment: roots-removed-during-gc

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.

Comment 1

[Andrew McCreight [:mccr8]]

Is this going to actually be called during the GC? Don't we do deferred finalization for these objects?

Comment 2

[Jon Coppeard (:jonco)]

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

Comment 3

[Jon Coppeard (:jonco)]

Attachment: roots-removed-alternate

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.

Comment 4

[Andrew McCreight [:mccr8]]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15c280b7d53b
Only repeat shutdown GCs for removed roots r=sfink

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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: "")

Comment 7

[Jon Coppeard (:jonco)]

We need to disable GC zeal in that test otherwise it risks being broken by anything that disturbs GC timing.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb92b29f5500
Only repeat shutdown GCs for removed roots r=sfink

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eb92b29f5500

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d413bd8d9dee
Disable gczeal in GC marking test r=me

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d413bd8d9dee