Closed
Bug 1099152
Opened 10 years ago
Closed 9 years ago
Investigate changing the GC API to allow incremental shrinking GCs
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: jonco, Assigned: jonco)
Details
Attachments
(4 files)
5.18 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
12.62 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
6.29 KB,
patch
|
terrence
:
review+
mccr8
:
review+
|
Details | Diff | Splinter Review |
14.61 KB,
patch
|
terrence
:
review+
mccr8
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(terrence)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(sphink)
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
Update the external API, splitting out JS::StartIncrementalGC() from JS::IncrementalGCSlice().
Attachment #8542988 -
Flags: review?(terrence)
Assignee | ||
Comment 6•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8542985 -
Flags: review?(terrence) → review+
Comment 7•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8542988 -
Flags: review?(terrence) → review+
Comment 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8542988 [details] [diff] [review] 3 - separate-start-incremental-api Requesting external review for the browser changes.
Attachment #8542988 -
Flags: review?(continuation)
Assignee | ||
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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 12•9 years ago
|
||
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+
Assignee | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/501a22044336 https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4e3b85c620 https://hg.mozilla.org/integration/mozilla-inbound/rev/969965f4c893 https://hg.mozilla.org/integration/mozilla-inbound/rev/e5a903979b5f
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)
Comment 15•9 years ago
|
||
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
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #15) Ah yes, I've run into this before too. Cheers.
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 17•9 years ago
|
||
Fixed and re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/24dae3ce853d https://hg.mozilla.org/integration/mozilla-inbound/rev/8b891ebcd21f https://hg.mozilla.org/integration/mozilla-inbound/rev/9192d432d87e https://hg.mozilla.org/integration/mozilla-inbound/rev/168e5b9bf198
Comment 18•9 years ago
|
||
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
Assignee | ||
Comment 19•9 years ago
|
||
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
Assignee | ||
Comment 20•9 years ago
|
||
Re-re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b7338b7658 https://hg.mozilla.org/integration/mozilla-inbound/rev/37bbd26f0870 https://hg.mozilla.org/integration/mozilla-inbound/rev/e07fc74ab87e https://hg.mozilla.org/integration/mozilla-inbound/rev/2f43b22848b0
https://hg.mozilla.org/mozilla-central/rev/a6b7338b7658 https://hg.mozilla.org/mozilla-central/rev/37bbd26f0870 https://hg.mozilla.org/mozilla-central/rev/e07fc74ab87e https://hg.mozilla.org/mozilla-central/rev/2f43b22848b0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•