Closed Bug 995284 Opened 6 years ago Closed 5 years ago

Make the GC more deterministic in the shell

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(1 file)

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.
Assignee: nobody → jcoppeard
Summary: Make the GC more deterministic → Make the GC more deterministic in the shell
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.)
Attachment #8471573 - Flags: review?(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.
Attachment #8471573 - Flags: review?(terrence) → review+
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
https://hg.mozilla.org/mozilla-central/rev/18b669a70e15
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.