1239099 - When triggering non-incremental GC, finish IGC first instead of segueing into a new GC

Status: NEW

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: finish_what_you_start-v0.diff

Currently, if we enter GCRuntime::gc while we are in the middle of another incremental GC, we enter the GC, then just before doing work we resetIncrementalGC. This should rewind our state as if we had not yet done anything for the GC. Unfortunately, this is hard to get right and we were not fully resetting the statistics state, causing us to unilaterally assert in debug builds when you minimize memory usage in about:memory while a GC is ongoingg. Whoops!

Even if we fix resetIncrementalGC, this plays havoc with GC reporting. It looks as if you have a single IGC with an enormous last slice that does too much work. Moreover, we're eating quite a bit of complexity to support this and it's not clear why we shouldn't just finish the ongoing GC first. I guess performance, but this should only happen with weirdo triggers like the minimize memory button.

Turns out we already had this change as part of bug 1202914. I'm splitting it off so that we can land in even smaller pieces. I think the bustage in the previous try run was from a different patch in the series, but got misattributed. In any case, there is a clean try run for this particular change:

   https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b694fc1d3f2&selectedJob=15312152

I'm carrying over the r+ from jonco in the previous bug and via chat this morning.

Comment 1

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29a5d0c6ea47ffb17df9bebc7da22df25e7eb451
Bug 1239099 - Finish ongoing GCs instead of segueing into another one; r=jonco

Comment 2

[Phil Ringnalda (:philor)]

Remember back in November, when you had to back out your patch for bug 1219498 because it caused a huge spike in bug 1093064, and caused a bunch of b2g debug "dom/cache/test/mochitest/test_cache_orphaned_cache.html | disk usage should have grown"?

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/98756a36223c for causing all of those again, and bug 1239206, and probably https://treeherder.mozilla.org/logviewer.html#?job_id=19709435&repo=mozilla-inbound, and... I dunno what all else, there's a lot of overlapping bustage.

Comment 3

[Phil Ringnalda (:philor)]

It's almost impossible to tell since devtools tests crash with a jseng signature more often than not, but i suspect the "devtools/client/netmonitor/test/browser_net_send-beacon-other-tab.js | application crashed [@ Interpret]" of https://treeherder.mozilla.org/logviewer.html#?job_id=19718241&repo=mozilla-inbound and friends is from this, too.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

The cache test does some explicit GC's in the test to reach some invariants in the cache code.  We could adjust the test to match new GC behavior if necessary.

I believe this is the explicit gc that is probably changing behavior in this test:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/test/mochitest/test_cache_orphaned_cache.html#92

That essentially just does SpecialPowers.exactGC(window):

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/test/mochitest/test_cache_orphaned_cache.html#55

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

The IDB tests also use SpecialPowers.exactGC():

  https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/helpers.js#321

Which probably explains the spike in bug 1093064.

Comment 6

[Terrence Cole [:terrence]]

(In reply to Phil Ringnalda (:philor) from comment #2)
> Remember back in November, when you had to back out your patch for bug
> 1219498 because it caused a huge spike in bug 1093064, and caused a bunch of
> b2g debug "dom/cache/test/mochitest/test_cache_orphaned_cache.html | disk
> usage should have grown"?

Nope!
 
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/98756a36223c for
> causing all of those again, and bug 1239206, and probably
> https://treeherder.mozilla.org/logviewer.html#?job_id=19709435&repo=mozilla-
> inbound, and... I dunno what all else, there's a lot of overlapping bustage.

Thanks for backing out! Odd: the try run was [what passes for] green these days.

(In reply to Ben Kelly [:bkelly] from comment #4)
> The cache test does some explicit GC's in the test to reach some invariants
> in the cache code.  We could adjust the test to match new GC behavior if
> necessary.
> 
> I believe this is the explicit gc that is probably changing behavior in this
> test:

Awesome! Thank you for the tip!

>  
> https://dxr.mozilla.org/mozilla-central/source/dom/cache/test/mochitest/
> test_cache_orphaned_cache.html#92
> 
> That essentially just does SpecialPowers.exactGC(window):
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/cache/test/mochitest/
> test_cache_orphaned_cache.html#55

I'll take a look.

Comment 7

[Phil Ringnalda (:philor)]

Yeah, I found that try run horrifying. It does have two of the things I backed you out for, the bug 1093064 thing in Mulet mochitest-2 and a test_cache_orphaned_body.html failure in emulator mochitest-3, but I doubt I would have noticed them.