Closed Bug 1099152 Opened 5 years ago Closed 5 years ago

Investigate changing the GC API to allow incremental shrinking GCs

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(4 files)

At the moment there is no way to trigger an incremental shrinking GC from the browser because JS::IncrementalGC() always specifies invocation kind GC_NORMAL.

The internal API GCRuntime::gcSlice() does take an invocation kind, but it allows you to pass a different one for each slice, which doesn't make any sense.

To change the APIs (both internal and external) to overcome these problems it seems that we need separate methods to initiate and continue incremental GCs, something like:

  GCRuntime::startIncrementalGC(JSGCInvocationKind gcKind)
  GCRuntime::gcSlice()
  
and

  JS::StartIncrementalGC(JSGCInvocationKind gcKind)
  JS::IncrementalGCSlice()

Obviously this would result in a reasonable amount of churn which is undesirable.

I've been in two minds as to do this but I think it's probably best way forward.  What do you guys think?
Flags: needinfo?(terrence)
Flags: needinfo?(sphink)
Strongly agree.
Flags: needinfo?(terrence)
I don't really know how this stuff is handled from Gecko, but in my quick skimming it seems like the callers already know whether they're starting a new incremental GC or continuing an already running one. That was my only concern -- do callers expect an API for "just do a little bit of GC work now"? It looks like they don't, and it's easy to check whether we're in an incremental GC anyway.

So in the end, I think this is totally worth the churn.
Flags: needinfo?(sphink)
Preparatory patch to change places where we do a non-incremental GC to call gc() rather than gcSlice().  In the shell this will do a non-incremental GC because GCRuntime::sliceBudget is unlimited by default, but in the browser it will start an incremental GC.

Adds a special reason for incremental GC slices triggered by maybeAllocTriggerZoneGC().
Attachment #8542985 - Flags: review?(terrence)
Separate GCRuntime::gcSlice() method into startGC() and gcSlice().  Rename gcFinalSlice() to finishGC() for similarity to startGC().

I considered calling these startIncrementalGC(), incrementalGCSlice() etc but thought that was too verbose, but feel free to disagree :)
Attachment #8542987 - Flags: review?(terrence)
Update the external API, splitting out JS::StartIncrementalGC() from JS::IncrementalGCSlice().
Attachment #8542988 - Flags: review?(terrence)
Finally, was can make the external APIs take the invocation kind directly, allowing us to start and incremental shrinking GC if we need to.
Attachment #8542998 - Flags: review?(terrence)
Attachment #8542985 - Flags: review?(terrence) → review+
Comment on attachment 8542987 [details] [diff] [review]
2 - separate-start-incremental-internal

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

::: js/src/jsgc.cpp
@@ +6408,4 @@
>  GCRuntime::gcDebugSlice(SliceBudget &budget)
>  {
>      if (!ZonesSelected(rt)) {
>          if (JS::IsIncrementalGCInProgress(rt))

While you're here: s/JS::Is/is/

@@ +6412,5 @@
>              JS::PrepareForIncrementalGC(rt);
>          else
>              JS::PrepareForFullGC(rt);
>      }
> +    if (!JS::IsIncrementalGCInProgress(rt))

And here: s/JS::Is/is/

@@ +6707,5 @@
>              // This triggers incremental GC but is actually ignored by IncrementalMarkSlice.
>              budget = SliceBudget(WorkBudget(1));
>          }
>  
> +        if (!JS::IsIncrementalGCInProgress(rt))

And here: s/JS::Is/is/
Attachment #8542987 - Flags: review?(terrence) → review+
Attachment #8542988 - Flags: review?(terrence) → review+
Comment on attachment 8542998 [details] [diff] [review]
4 - pass-invocation-kind-to-external-apis

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

This series is a huge improvement; we should have done it ages ago.

::: js/public/GCAPI.h
@@ +38,5 @@
> +    /* Normal invocation. */
> +    GC_NORMAL           = 0,
> +
> +    /* Minimize GC triggers and release empty GC chunks right away. */
> +    GC_SHRINK             = 1

It looks like the spacing around the = is different between these two; just use one space on each side.

::: js/src/jsfriendapi.cpp
@@ +203,4 @@
>  }
>  
>  JS_FRIEND_API(void)
> +JS::StartIncrementalGC(JSRuntime *rt, JSGCInvocationKind gckind, gcreason::Reason reason, int64_t millis)

These two methods are part of our explicit embedding API: they should be defined in jsgc and not jsfriendapi. We should move all of these at once in a followup bug.
Attachment #8542998 - Flags: review?(terrence) → review+
Comment on attachment 8542988 [details] [diff] [review]
3 - separate-start-incremental-api

Requesting external review for the browser changes.
Attachment #8542988 - Flags: review?(continuation)
Comment on attachment 8542998 [details] [diff] [review]
4 - pass-invocation-kind-to-external-apis

Requesting external review for the browser changes.
Attachment #8542998 - Flags: review?(continuation)
Comment on attachment 8542988 [details] [diff] [review]
3 - separate-start-incremental-api

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

r=me for the nsJSEnvironment changes
Attachment #8542988 - Flags: review?(continuation) → review+
Comment on attachment 8542998 [details] [diff] [review]
4 - pass-invocation-kind-to-external-apis

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

r=me for the browser changes
Attachment #8542998 - Flags: review?(continuation) → review+
All four (and the followup in 3d195c54dcf1) backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7cd513b565 for hazard failures:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5034795&repo=mozilla-inbound
Flags: needinfo?(jcoppeard)
Looks like part 2 ran into the same thing I did in bug 1084651: You changed the signature of GCRuntime::collect, and now the hazard analysis can't find it. You'll need to change the signature in [1]. I think |void js::gc::GCRuntime::collect(uint8, js::SliceBudget, uint32)| will probably work, though you might have to check with sfink.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/devtools/rootAnalysis/loadCallgraph.js#163
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #15)
Ah yes, I've run into this before too.  Cheers.
Flags: needinfo?(jcoppeard)
Backed out for causing frequent B2G debug emulator connection timeouts as shown in the log below. They started on your push, and more damningly, I went back to last Friday and saw the same failures starting with the push then and going away with the backout.
https://hg.mozilla.org/integration/mozilla-inbound/rev/06ec26af14f3

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5075431&repo=mozilla-inbound
Removed parts of the first patch that turned incremental browser GCs into non-incremental ones.

Try run is green: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7bac049c160e
You need to log in before you can comment on or make changes to this bug.