Closed Bug 737075 Opened 8 years ago Closed 8 years ago
unmark gray observers implemented in JS held by observer service
This seems like a major chunk of the remaining nodes in the CC graph with a small tab set. I see JS implementations of nsIBlockListService, nsILoginManagerStorage, nsIUrlClassifierHashCompleter, nsIUrlListManager, nsIContentPrefService and other things in the CC graph. These seem to mostly be nsIObservers as well, so maybe they are being held live by the observer service. This would require adding a new method to the observer service that iterates over all registered observers, QIs them to wrapped JS, then unmarks gray on them if it succeeds. This new method would be invoked from nsCCUncollectableMarker::Observe under cleanupJS.
One thing I'm worried about here is that the decref that is happening on all listeners may cause additional things to get added to the purple buffer. I feel like I'm seeing some things in the graph with this patch that I'm not seeing without it. But I should check carefully to see if that's really true.
Hmm. Though if the QI fails, then it just returns null, so hopefully it doesn't generate refcount traffic, so my concern is likely misplaced. I don't care about refcount traffic on nsXPCWrappedJS.
Assignee: nobody → continuation
Attachment #607283 - Attachment is obsolete: true
This appears to be leaky somehow.
Comment on attachment 610281 [details] [diff] [review] fix getter to avoid leak The main weird part here is xpc_TryUnmarkWrappedGrayObject, which attempts to QI elem to a wrapped JS. If that succeeds, then it floods black over any gray objects.
Attachment #610281 - Flags: review?(benjamin)
Comment on attachment 610281 [details] [diff] [review] fix getter to avoid leak I'm probably not the correct person to be doing the final review on this, because I don't really know the cycle collector. Does xpc_TryUnmarkWrappedGrayObject make it so the cycle collector will never consider this object to be part of a collectable cycle? "unmark" seems like an odd name for that behavior given the normal mark/sweep terminology for a garbage collector. Since you have access to the nsObserverList*, why are you bothering with the XPCOM goop ->GetObserverList, instead of just looping over mObservers directly? Also, some observers are weak observers: I believe that you should only be doing this action on the strong observer refs, since we very well might want to remove the weakly-referenced observers during cycle collection. Finally, this is an implementation detail of the observer service and should not be on the nsIObserverService interface. Can you just add a C++ method on nsObserverService and call it directly from nsCCUncollectableMarker::Observe? You should be able to static_cast from mozilla::services::GetObserverService to nsObserverService.
Attachment #610281 - Flags: review?(benjamin) → review-
Here's a new version that mostly addresses bsmedberg's review comments. The remaining problem I am having is that in order to cast to nsObserverService in nsCCUncollectableMarker I have to include nsObserverService.h, which in turn includes nsObserverList.h, which it can't find. I guess the header isn't being exported somehow? I'll try just wrapping up the whole process of getting the observer, casting it and calling the unmark method on it somewhere in XPCOM land that I can call without exposing the internals of nsObserverService.
Attachment #610281 - Attachment is obsolete: true
Though maybe that's just a bug with nsObserverService.h, as it seems silly to export a header that you can't use.
With the new more correct unmarking, I see a few nsIObservers still in the CC graph. Specifically, nsIUrlClassifierHashCompleter and nsIPrivateBrowsingService. Their implementations are fairly small (< 10 JS objects it looks like), so it isn't a big deal.
All those Moths failures look a little sketchy, but I think I just got unlucky. https://tbpl.mozilla.org/?tree=Try&rev=2832f51fae34
Comment on attachment 619592 [details] [diff] [review] unmark gray strongly held observers > >+ // Unmark gray any strongly held observers implemented in JS so the >+ // cycle collector will not traverse them. >+ void UnmarkGrayStrongObservers(); A bit strange comment. Perhaps drop 'gray' from the comment "Unmar any strongly..." >+static PLDHashOperator >+UnmarkGrayObserverEntry(nsObserverList *aObserverList, void *aClosure) Here and elsewhere I prefer Foo* aFoo
Attachment #619592 - Flags: review?(bugs) → review+
Thanks for the reviews. https://hg.mozilla.org/integration/mozilla-inbound/rev/0e2658794e06
Target Milestone: --- → mozilla15
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Should we think of landing this to FF14?
I think this should ride the trains: it provides perf benefits but not correctness/stability benefits, right?
Yeah, there's no big hurry for this. The case it helps the most is when there are only a few tabs open and no leaks, in which case CC times aren't that bad anyways.
You need to log in before you can comment on or make changes to this bug.