995284 - Make the GC more deterministic in the shell

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Currently the GC uses a couple of time-based heuristics:

http://dxr.mozilla.org/mozilla-central/search?q=PRMJ_Now+path%3Ajs%2Fsrc%2Fjsgc.cpp&case=true&redirect=true

These can be disabled bu building with --enable-more-deterministic, however that changes other engine behaviour too.

It would be great if the shell was entirely deterministic.  This may be possible by only enabling time-based heuristics in the browser, or by replacing them with heuristics based on GC number instead.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug995284-deterministic-gc

There are two places this affected us:

1) In MaybeGC(), I split off the time-based periodic full GC so that it is only ever triggered when JS_MaybeGC() is called from the browser, so it's not called any more in the shell.

2) For releaseObservedTypes(), I changed this to happen based on a count of the number of major GCs rather than the time as I figured we probably do want to this to happen in the shell. 

The code here was a bit broken anyway as this method got called for every zone group swept in an incremental GC, and would have returned true only for the first one.  I've changed this so that when it triggers observed types are released for all zone groups in that GC.

(This means it's still a bit broken as we may be doing a zone GC when it triggers and not release the types for the other zones... Maybe this should wait until a full GC happens before triggering, but I'm unsure whether we guarantee these will occur or how often.  This is a side issue to this bug though.)

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8471573 [details] [diff] [review]
bug995284-deterministic-gc

Review of attachment 8471573 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/GCRuntime.h
@@ +589,5 @@
>       */
>      volatile uintptr_t    isNeeded;
>  
> +    /* Incremented at the start of every major GC. */
> +    uint64_t              majorGCNumber;

Great! We were going to need this to do zone GC's in CC_WAITING too, so it's nice to have it already done.

::: js/src/jsgc.cpp
@@ +2468,5 @@
>              numArenasFreeCommitted > decommitThreshold)
>          {
>              JS::PrepareForFullGC(rt);
>              GCSlice(rt, GC_SHRINK, JS::gcreason::MAYBEGC);
>          } else {

I guess in practice the CC_WAITING timer always fires ahead of this timer. This seems like a poor way to backstop our heap. I think if we're going to accept non-determinism anyway we should be directly checking the available allocable space that the OS is reporting and using that to trigger instead of a random timeout unmoored from our actual usage.

Comment 3

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6970e7f17e1

Comment 4

[Ed Morley [:emorley]]

Reverted for crashes on OS X and Windows:
https://tbpl.mozilla.org/php/getParsedLog.php?id=45827775&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=45828422&tree=Mozilla-Inbound
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f6970e7f17e1

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/76b55c0850ca

Comment 5

[Ed Morley [:emorley]]

Also Linux x64
https://tbpl.mozilla.org/php/getParsedLog.php?id=45828790&tree=Mozilla-Inbound

Comment 6

[Jon Coppeard (:jonco)]

Removed bogus assertion and relanded - we may miss our target GC in which to release observed types due to resets.

https://hg.mozilla.org/integration/mozilla-inbound/rev/18b669a70e15

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18b669a70e15