Closed Bug 899367 Opened 6 years ago Closed 6 years ago

Stop Cycle Collecting JSContexts

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 2 obsolete files)

8.30 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
1.72 KB, patch
billm
: review+
Details | Diff | Splinter Review
6.69 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
7.45 KB, patch
mccr8
: review+
khuey
: feedback+
Details | Diff | Splinter Review
4.97 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
17.55 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
3.83 KB, patch
mccr8
: review+
luke
: review+
Details | Diff | Splinter Review
5.48 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
1.78 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
17.55 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
5.41 KB, patch
luke
: review+
Details | Diff | Splinter Review
7.32 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
2.16 KB, patch
Details | Diff | Splinter Review
Andrew and I just spent a little while discussing some of the CC goop surrounding JSContexts. I think it can all be simplified immensely.

My understanding of the current setup is as follows:

* During GC, we iterate over all JSContexts that have JSOPTION_UNROOTED_GLOBAL set, and mark their default compartment object as gray. Given our usage of context callbacks, this seems to be set for every JSContext in the main thread JSRuntime.

* During CC, we note an edge from the JSContext (reported as a node with an artificial refcount to the cycle collector) to the default global object. We bump this refcount to 2 if there are outstanding requests on the cx (this should be unnecessary after bug 860085). We then indicate an edge from the nsJSContext (during it's traverse) to the JSContext via nsContentUtils::XPConnect()->NoteJSContext.


This setup used to be more interesting when the default compartment object had semantic meaning within the JS engine. But now that it's just an opaque (though occasionally traced) object, the only thing we get out of this deal is the ability to iterate over all the nsScriptContexts in the system (via the JSContext iterator). But that can just as easily by done by the outer window hashmap.

So I think we can just cut JSContext out of the loop, do the gray marking via the hashmap, and trace directly from nsJSContext to the outer window.
> Given our usage of context callbacks, this seems to be set for every JSContext in the main thread JSRuntime.
It should be okay even if it isn't true for all JSContexts, because JSContexts that don't have that flag set are rooted by the JS engine, and thus XPConnect doesn't need to root them itself.  If I understand that correctly.
cool
Does anyone understand why XPConnect does not use rooted globals?
If it used rooted globals, then all the globals would be marked black.
Until when?  Forever?  How does a global get collected without unrooted globals?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5)
> Until when?  Forever?  How does a global get collected without unrooted
> globals?

The global would only be collected when you destroy the context.
Ok, so that's not really any different than what XPConnect does today, except that the global gets marked gray instead of black.
Oh, yes, I mis-spoke. I didn't mean "all the globals everywhere", just all the ones that are used as default compartment objects for contexts.
(In reply to Bill McCloskey (:billm) from comment #8)
> Oh, yes, I mis-spoke. I didn't mean "all the globals everywhere", just all
> the ones that are used as default compartment objects for contexts.

Yes. And keep in mind that, in general, these aren't actually globals at all. They're outer window proxies.
Depends on: 901106
Blocks: 908981
Green! Uploading patches and flagging for review.
We don't cycle collect any other kind, so there's no difference between marking
them gray and letting the JS engine mark them black. This also gets rid of that
nasty code which reset the flag, which billm theorizes has to do with faulty
logic in ContextHolder when creating ephemeral sandbox cxes. The assertion in
this patch should catch us if anything goes wrong.
Attachment #795068 - Flags: review?(continuation)
This allows us to assert that certain callbacks get invoked during the gray
phase of a marking GC.
Attachment #795069 - Flags: review?(continuation)
Note that this machinery will go away in further patches. But we want to be
deliberate about each of these steps.
Attachment #795071 - Flags: review?(continuation)
We need this explicit API because otherwise the default compartment object will
get set implicitly during the first call to InitClasses.
Attachment #795072 - Flags: review?(luke)
Attachment #795072 - Flags: review?(continuation)
We need to simultaneously report the new Heap Pointer to the cycle collector,
and remove the old machinery for cycle-collecting the default object via the
JSContext.
Attachment #795075 - Flags: review?(continuation)
\o/
Attachment #795076 - Flags: review?(continuation)
Default compartment objects are never gray anymore.
Attachment #795077 - Flags: review?(continuation)
Some of the uses of this are a bit nonsensical now, but that's a problem for
another day.
Attachment #795079 - Flags: review?(continuation)
I imagine this might be something smaug is interested in as well. Olli, I flagged mccr8 because I think your review load tends to be heavier, but I of course welcome any feedback you have time to give on these patches. NEEDINFOing just to make sure this is on your radar. :-)
Flags: needinfo?(bugs)
Comment on attachment 795068 [details] [diff] [review]
Part 1 - Only use JSOPTION_UNROOTED_GLOBAL for DOM JSContexts. v1

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

This is a lot nicer!

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +545,5 @@
> +    // DOM JSContexts are the only JSContexts that cycle-collect their default
> +    // compartment object, so they're the only ones that we need to do the
> +    // JSOPTION_UNROOTED_GLOBAL dance for. The other ones are just marked black.
> +    MOZ_ASSERT(js::HasUnrootedGlobal(acx) == !!GetScriptContextFromJSContext(acx));
> +    if (!js::HasUnrootedGlobal(acx))

nit: this isn't XPConnect, so brace this "if"

@@ +1212,5 @@
>    CustomGCCallback(aStatus);
>  }
>  
>  bool
>  CycleCollectedJSRuntime::OnContext(JSContext* aCx, unsigned aOperation)

OnContext is pointless now.  We need to do something like make OnContext virtual, and change the CustomContextCallback definition in XPConnect to an OnContext override.  I can fix that in a followup if you'd like.
Attachment #795068 - Flags: review?(continuation) → review+
Comment on attachment 795070 [details] [diff] [review]
Part 3 - Explicitly trace outer windows, rather than doing it via JSContext iteration. v1

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

::: dom/base/nsJSEnvironment.h
@@ +181,5 @@
>    nsJSContext *mNext;
>    nsJSContext **mPrev;
>  
> +  /* This function needs access to the linked list members above. */
> +  friend void mozilla::dom::TraceOuterWindows(JSTracer *aTracer);

Maybe just make TraceOuterWindows a static method on nsJSEnvironment to avoid this?
Attachment #795070 - Flags: review?(continuation) → review+
Comment on attachment 795071 [details] [diff] [review]
Part 4 - Explicitly traverse outer windows, rather than doing it via JSContext iteration. v1

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

It is nice how this mainthread-only stuff disappears from CycleCollectedJSRuntime.

::: dom/base/nsJSEnvironment.cpp
@@ +2927,5 @@
> +    if (cx->mContext &&
> +        (outer = js::DefaultObjectForContextOrNull(cx->mContext)) &&
> +        xpc_IsGrayGCThing(outer))
> +    {
> +      aCb.NoteNativeRoot(cx->mContext, mozilla::CycleCollectedJSRuntime::JSContextParticipant());

Maybe hoist this call to JSContextParticipant() out of the loop?

::: dom/base/nsJSEnvironment.h
@@ +184,5 @@
>    nsJSContext **mPrev;
>  
>    /* This function needs access to the linked list members above. */
>    friend void mozilla::dom::TraceOuterWindows(JSTracer *aTracer);
> +  friend void mozilla::dom::TraverseOuterWindows(nsCycleCollectionNoteRootCallback& aCb);

As with the other one, consider just making this a static method on nsJSEnvironment.
Attachment #795071 - Flags: review?(continuation) → review+
Comment on attachment 795071 [details] [diff] [review]
Part 4 - Explicitly traverse outer windows, rather than doing it via JSContext iteration. v1

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

Kyle, do workers really not use any of this for context iteration?  I forget how you ended up fixing that problem...
Attachment #795071 - Flags: feedback?(khuey)
Comment on attachment 795072 [details] [diff] [review]
Part 5 - Add an API to opt out of default compartment objects. v1

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

I don't really know enough about what default compartment objects are to review this.

::: js/src/vm/SelfHosting.cpp
@@ +688,5 @@
>  JSRuntime::initSelfHosting(JSContext *cx)
>  {
>      JS_ASSERT(!selfHostingGlobal_);
> +
> +    bool haveDefaultObject = cx->hasOption(JSOPTION_NO_DEFAULT_COMPARTMENT_OBJECT);

This code seems weird to me.  We have a default object if the flag that says we have no default object is set?

@@ +696,5 @@
>                                                    NULL, JS::DontFireOnNewGlobalHook)))
>          return false;
>      JSAutoCompartment ac(cx, selfHostingGlobal_);
> +    if (haveDefaultObject)
> +        js::SetDefaultObjectForContext(cx, selfHostingGlobal_);

Shouldn't we set a default object is we don't have a default object?
Attachment #795072 - Flags: review?(continuation)
Comment on attachment 795069 [details] [diff] [review]
Part 2 - Add an API to determine if we're marking gray. v1

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

A JS person should look at this, as it has been quite some time since I've really delved into the GC code.

This is used in part 3, in nsJSEnvironment.cpp

::: js/src/jsapi.cpp
@@ +2409,5 @@
>  
> +extern JS_PUBLIC_API(bool)
> +JS_IsMarkingGray(JSTracer *trc)
> +{
> +    return trc->callback == GCMarker::GrayCallback;

Please assert |JS_IsGCMarkingTracer(trc)|, as this doesn't really make any sense for a non-GCMarking tracer.
Attachment #795069 - Flags: review?(continuation) → review?(wmccloskey)
Comment on attachment 795073 [details] [diff] [review]
Part 6 - Stop depending on the default compartment object for DOM JSContexts. v1

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

::: js/jsd/jsd_xpc.cpp
@@ +1694,5 @@
>  NS_IMETHODIMP
>  jsdContext::GetGlobalObject (jsdIValue **_rval)
>  {
>      ASSERT_VALID_EPHEMERAL;
> +    nsIScriptContext* scx = GetScriptContextFromJSContext(mJSCx);

Can you combine this logic ("is there a scx? if so, get native global else get default object") into a function, as you are using it three times in this patch.
Attachment #795073 - Flags: review?(continuation) → review+
Comment on attachment 795074 [details] [diff] [review]
Part 7 - Have nsGlobalWindow go through nsJSContext to access the outer. v1

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

::: dom/base/nsGlobalWindow.cpp
@@ +2046,5 @@
>  nsGlobalWindow::SetOuterObject(JSContext* aCx, JS::Handle<JSObject*> aOuterObject)
>  {
>    JSAutoCompartment ac(aCx, aOuterObject);
>  
> +  // Inform the nsJSContext, which is the canoncial holder of the outer.

nit: canonical

::: dom/base/nsJSEnvironment.cpp
@@ +2561,5 @@
>    }
>  }
>  
>  void
> +nsJSContext::SetWindowProxy(JS::Handle<JSObject*> aWindowProxy)

I don't think this needs to be a handle, but it doesn't matter as far as I can tell.
Attachment #795074 - Flags: review?(continuation) → review+
Comment on attachment 795075 [details] [diff] [review]
Part 8 - Stop storing the window proxy on the JSContext. v1

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

::: dom/base/nsJSEnvironment.cpp
@@ +913,5 @@
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsJSContext)
>    NS_ASSERTION(!tmp->mContext || !js::ContextHasOutstandingRequests(tmp->mContext),
>                 "Trying to unlink a context with outstanding requests.");
>    tmp->mIsInitialized = false;
>    tmp->mGCOnDestruction = false;

You need to clear mWindowProxy in UNLINK to ensure any cycles through that pointer are broken.

::: dom/base/nsJSEnvironment.h
@@ -186,5 @@
>    nsJSContext *mNext;
>    nsJSContext **mPrev;
>  
> -  /* This function needs access to the linked list members above. */
> -  friend void mozilla::dom::TraceOuterWindows(JSTracer *aTracer);

okay, well, ignore my suggestions above on these methods you immediately remove...

::: js/xpconnect/idl/nsIXPConnect.idl
@@ -564,5 @@
> -     * @param aJSContext The JSContext to note.
> -     * @param aCb The cycle collection traversal callback.
> -     */
> -    [noscript,notxpcom] void noteJSContext(in JSContextPtr aJSContext,
> -                                           in nsCCTraversalCallbackRef aCb);

You can remove the declaration of nsCCTraversalCallbackRef and nsCycleCollectionTraversalCallback earlier in this file, looks like.  While you are there, please also remove the declarations of nsScriptObjectTracerPtr and nsScriptObjectTracer.
Attachment #795075 - Flags: review?(continuation) → review+
Comment on attachment 795076 [details] [diff] [review]
Part 9 - Remove JSOPTION_UNROOTED_GLOBAL. v1

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

Woo!

This should get a JS peer review, too, even though it is only removing some stuff only used for the browser.
Attachment #795076 - Flags: review?(luke)
Attachment #795076 - Flags: review?(continuation)
Attachment #795076 - Flags: review+
Comment on attachment 795077 [details] [diff] [review]
Part 10 - Remove xpc_UnmarkGrayContext. v1

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

Nice

::: dom/base/nsJSEnvironment.cpp
@@ +1127,3 @@
>  nsJSContext::GetNativeGlobal()
>  {
>    return mWindowProxy;

I'm not sure where in the patch stack it should go (probably when you add GetWindowProxy()), but I think GetWindowProxy() should call UnmarkGrayObject() on mWindowProxy.  If you are "activating" a nsJSContext for the first time after a GC, the window proxy should be black.  Hopefully that's the only way you can start touching the window proxy.  Fortunately the TRACE thing for mWindowProxy doesn't go through GetWindowProxy(), so you don't have to add a GetWindowProxyPreserveColor()

Eventually, we'll hopefully be able to make the Heap<JSObject*> class automatically do that on a get() to avoid that nonsense.
Attachment #795077 - Flags: review?(continuation) → review+
Attachment #795078 - Flags: review?(continuation) → review+
GetNativeGlobal() should also UnmarkGray, but you are deleting it, so whatever.
Comment on attachment 795079 [details] [diff] [review]
Part 12 - Make GetNativeGlobal consumers call GetWindowProxy and rm the former. v1

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

::: dom/base/nsJSEnvironment.cpp
@@ +1073,5 @@
>  nsIScriptGlobalObject *
>  nsJSContext::GetGlobalObject()
>  {
>    AutoJSContext cx;
> +  JS::Rooted<JSObject*> global(mContext, mWindowProxy);

This needs to either use GetWindowProxy() or UnmarkGrayObject().

@@ +1165,5 @@
>    pusher.Push(mContext);
>  
>    Maybe<nsRootedJSValueArray> tempStorage;
>  
> +  JS::Rooted<JSObject*> global(mContext, mWindowProxy);

Same as above.

@@ +1461,5 @@
>  
>        AutoFree iidGuard(iid); // Free iid upon destruction.
>  
>        nsCOMPtr<nsIXPConnectJSObjectHolder> wrapper;
> +      JS::Rooted<JSObject*> global(cx, xpc_UnmarkGrayObject(mWindowProxy));

Might as well make this consistent with the previous two cases, whatever you decide.
Attachment #795079 - Flags: review?(continuation) → review+
My basic theory here is that before we were careful to mark the window proxies of JSContexts black when we started touching the JSContexts, but now we no longer do that, so we have to make sure that any time we grab the window proxy itself we need to make sure it is black.
Comment on attachment 795077 [details] [diff] [review]
Part 10 - Remove xpc_UnmarkGrayContext. v1

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

::: js/xpconnect/src/nsXPConnect.cpp
@@ +1314,5 @@
>  JSContext*
>  nsXPConnect::GetCurrentJSContext()
>  {
>      JSContext *cx = GetRuntime()->GetJSContextStack()->Peek();
> +    return cx;

No need for the local variable here
Comment on attachment 795078 [details] [diff] [review]
Part 11 - Remove check for non-WindowProxy objects on an scx. v1

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

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +878,5 @@
>    JSContext* iter = nullptr;
>    JSContext* cx;
>    JSAutoRequest ar(nsContentUtils::GetSafeJSContext());
>    while ((cx = JS_ContextIterator(mJSRuntime, &iter))) {
> +    // Skip anything without an nsIScriptContext.

Maybe "Skip anything that doesn't have an outer window"?
Comment on attachment 795072 [details] [diff] [review]
Part 5 - Add an API to opt out of default compartment objects. v1

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

::: js/src/vm/SelfHosting.cpp
@@ +696,5 @@
>                                                    NULL, JS::DontFireOnNewGlobalHook)))
>          return false;
>      JSAutoCompartment ac(cx, selfHostingGlobal_);
> +    if (haveDefaultObject)
> +        js::SetDefaultObjectForContext(cx, selfHostingGlobal_);

Seems like the 'have' is misleading in this context; how about 'receivesDefaultObject'.  (Also, agreed with Andrew, seems like the initialization should be negated.)
Comment on attachment 795076 [details] [diff] [review]
Part 9 - Remove JSOPTION_UNROOTED_GLOBAL. v1

My favorite kind of review.
Attachment #795076 - Flags: review?(luke) → review+
Blocks: 909436
(In reply to Andrew McCreight [:mccr8] from comment #25)
> OnContext is pointless now.  We need to do something like make OnContext
> virtual, and change the CustomContextCallback definition in XPConnect to an
> OnContext override.  I can fix that in a followup if you'd like.

Filed bug 909436.(In reply to Andrew McCreight [:mccr8] from comment #26)

> Maybe just make TraceOuterWindows a static method on nsJSEnvironment to
> avoid this?

These functions are ephemeral to the patch stack, so I don't think we need to bother.
Attachment #795072 - Attachment is obsolete: true
Attachment #795072 - Flags: review?(luke)
Attachment #795569 - Flags: review?(luke)
(In reply to Andrew McCreight [:mccr8] from comment #31)
> Can you combine this logic ("is there a scx? if so, get native global else
> get default object") into a function, as you are using it three times in
> this patch.

Done. I didn't use the new helper in nsCxPusher though, because we already a member variable with the result of the GetScriptContextFromJSContext dance, and there's no sense in doing that work twice in such a hot function.
Comment on attachment 795569 [details] [diff] [review]
Part 5 - Add an API to opt out of default compartment objects. v2

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

::: js/src/vm/SelfHosting.cpp
@@ +690,5 @@
>      JS_ASSERT(!selfHostingGlobal_);
> +
> +    bool receivesDefaultObject = !cx->hasOption(JSOPTION_NO_DEFAULT_COMPARTMENT_OBJECT);
> +    RootedObject savedGlobal(cx, receivesDefaultObject
> +                                   ? js::DefaultObjectForContextOrNull(cx) : NULL);

SM ternary indention style is:
RootedObject savedGlobal(cx, receivesDefaultObject
                             ? js::DefaultObjectForContextOrNull(cx)
                             : NULL);
Attachment #795569 - Flags: review?(luke) → review+
Comment on attachment 795071 [details] [diff] [review]
Part 4 - Explicitly traverse outer windows, rather than doing it via JSContext iteration. v1

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

Yeah workers don't use this.
Attachment #795071 - Flags: feedback?(khuey) → feedback+
Comment on attachment 795069 [details] [diff] [review]
Part 2 - Add an API to determine if we're marking gray. v1

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

I agree with mccr8 about the assertion. It would also be nice to make this DEBUG-only. I don't like the idea of someone dynamically testing this.
Attachment #795069 - Flags: review?(wmccloskey) → review+
Addressed all review comments, and pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=54d94bc4a4fd
This was also causing test slowdowns on all platforms as made visible in the log below:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27091858&tree=Mozilla-Inbound
Depends on: 910126
And damn am I glad I split that little change into its own patch. This is the culprit rev:

https://hg.mozilla.org/try/rev/1327167eb9ed

This change is mostly a no-op refactor, but includes a call to xpc_UnmarkGrayObject in GetWindowProxy (as requested by Andrew in review). It seems pretty obvious now that the problem is that we're unmarking gray too often (and boy would that have been a pain to track down).

I'll try running bc tests tomorrow and see who's invoking GetWindowProxy on all the almost-dead DOMWindows.
My guess was right. We invoke GetWindowProxy() in CycleCollectedJSContext::
UsefulToMergeZones(), which means that we end up marking every single outer DOM
window black right before cycle collection. This causes us to leak the world.

I'm attaching an updated part 7 that does the obvious GetWindowProxyPreserveColor.
Flagging mccr8 for review. I'll also attach an interdiff from the previous part 7.
Attachment #795074 - Attachment is obsolete: true
Attachment #799172 - Flags: review?(continuation)
Attached patch Part 7 interdiffSplinter Review
Comment on attachment 799172 [details] [diff] [review]
Part 7 - Have nsGlobalWindow go through nsJSContext to access the outer. v3

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

Oops, I should have thought of that.
Attachment #799172 - Flags: review?(continuation) → review+
Flags: needinfo?(bugs)
(In reply to Bobby Holley (:bholley) from comment #59)
> https://tbpl.mozilla.org/?tree=Try&rev=ef6ad768802e

Looks like the windows bc oranges are fixed. Let's try a full run:

https://tbpl.mozilla.org/?tree=Try&rev=17d889dc1b3b
And a followup bustage fix for some functions that were renamed in the interim since the try push in comment 61:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a86fde943e6
(In reply to Joel Maher (:jmaher) from comment #65)
> This appears to have caused a TART regression on inbound

That was actually caused by bug 904533 (see bug 904533 comment 43 or the dev-tree-management thread).
Here's a bit of a curiosity: we've always had this weird extra shutdown CC that contains some JS and not much else that doesn't free anything, but that CC graph seems to be completely empty now.  I think that's due to this patch making it so we don't have these JSContexts lingering around holding things alive.  Kind of weird, but it is nice to see that a CC that does nothing also has nothing in the graph.
(In reply to Andrew McCreight [:mccr8] from comment #67)
> Here's a bit of a curiosity: we've always had this weird extra shutdown CC
> that contains some JS and not much else that doesn't free anything, but that
> CC graph seems to be completely empty now.  I think that's due to this patch
> making it so we don't have these JSContexts lingering around holding things
> alive.  Kind of weird, but it is nice to see that a CC that does nothing
> also has nothing in the graph.

Cool! Does that mean we can kill that last CC and assert that the CC graph is empty at that point? Or is that too brittle?
Having the extra CCs is nice when you are looking for leaks, and an extra GC+CC in debug builds isn't a huge deal (as the CC itself will take no time).  It might be nice to assert that the graph is empty, but I'm not sure offhand how to do that.  I'm already asserting that we don't do more than 2 shutdown CCs.
(In reply to Andrew McCreight [:mccr8] from comment #69)
> Having the extra CCs is nice when you are looking for leaks, and an extra
> GC+CC in debug builds isn't a huge deal (as the CC itself will take no
> time).  It might be nice to assert that the graph is empty, but I'm not sure
> offhand how to do that.  I'm already asserting that we don't do more than 2
> shutdown CCs.

It might require a bit more engineering, but presumably you can hook into whatever metrics you used to determine this. I think it's always great to assert when we get leak-related things to zero, since it prevents them from creeping up again.
You need to log in before you can comment on or make changes to this bug.