Last Comment Bug 737075 - unmark gray observers implemented in JS held by observer service
: unmark gray observers implemented in JS held by observer service
Status: RESOLVED FIXED
[Snappy:P2]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on: 738700
Blocks: 716598 722715
  Show dependency treegraph
 
Reported: 2012-03-19 10:07 PDT by Andrew McCreight [:mccr8]
Modified: 2012-06-18 14:31 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
hacky WIP (3.40 KB, patch)
2012-03-19 13:32 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Review
less hacky patch (3.69 KB, patch)
2012-03-26 09:52 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Review
fix getter to avoid leak (3.80 KB, patch)
2012-03-28 13:38 PDT, Andrew McCreight [:mccr8]
benjamin: review-
Details | Diff | Review
addressing review comments, WIP (5.10 KB, patch)
2012-04-25 15:54 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Review
unmark gray strongly held observers (5.59 KB, patch)
2012-04-25 17:20 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Review
unmark gray strongly held observers (5.60 KB, patch)
2012-04-30 09:29 PDT, Andrew McCreight [:mccr8]
benjamin: review+
bugs: review+
Details | Diff | Review

Description Andrew McCreight [:mccr8] 2012-03-19 10:07:26 PDT
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.
Comment 1 Andrew McCreight [:mccr8] 2012-03-19 13:32:33 PDT
Created attachment 607283 [details] [diff] [review]
hacky WIP
Comment 2 Andrew McCreight [:mccr8] 2012-03-23 11:20:41 PDT
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.
Comment 3 Andrew McCreight [:mccr8] 2012-03-23 17:29:07 PDT
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.
Comment 4 Andrew McCreight [:mccr8] 2012-03-26 09:52:15 PDT
Created attachment 609356 [details] [diff] [review]
less hacky patch
Comment 5 Andrew McCreight [:mccr8] 2012-03-26 16:53:45 PDT
This appears to be leaky somehow.
Comment 6 Andrew McCreight [:mccr8] 2012-03-28 13:38:57 PDT
Created attachment 610281 [details] [diff] [review]
fix getter to avoid leak
Comment 7 Andrew McCreight [:mccr8] 2012-03-29 14:05:10 PDT
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.
Comment 8 Benjamin Smedberg [:bsmedberg] 2012-03-30 09:10:13 PDT
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.
Comment 9 Andrew McCreight [:mccr8] 2012-03-30 09:57:16 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> 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.

I can get Olli to review it, too.

> Does
> xpc_TryUnmarkWrappedGrayObject make it so the cycle collector will never
> consider this object to be part of a collectable cycle?

Yes, at least until the next garbage collection.

> "unmark" seems like
> an odd name for that behavior given the normal mark/sweep terminology for a
> garbage collector.

It is indeed odd terminology.  Javascript objects have two bits, a black bit and a gray bit.  The black bit being set means the object is alive (or rather, was alive at the last time we did GC) and the gray bit being set means that the object can be considered as possible garbage by the CC.  UnmarkGray clears the gray bit, making it so that the CC will not consider freeing it.

> Since you have access to the nsObserverList*, why are you bothering with the
> XPCOM goop ->GetObserverList, instead of just looping over mObservers
> directly?

No good reason.  I'll look at how to do that.

> 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.

That's a good point.  I'll look into that.

> 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.

That sounds like a good idea.  It seemed kind of gross to have it there.

Thanks for all the comments!
Comment 10 Andrew McCreight [:mccr8] 2012-04-25 15:54:55 PDT
Created attachment 618468 [details] [diff] [review]
addressing review comments, WIP

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.
Comment 11 Andrew McCreight [:mccr8] 2012-04-25 16:09:30 PDT
Though maybe that's just a bug with nsObserverService.h, as it seems silly to export a header that you can't use.
Comment 12 Andrew McCreight [:mccr8] 2012-04-25 17:20:39 PDT
Created attachment 618488 [details] [diff] [review]
unmark gray strongly held observers
Comment 13 Andrew McCreight [:mccr8] 2012-04-26 11:34:43 PDT
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.
Comment 14 Andrew McCreight [:mccr8] 2012-04-26 21:55:37 PDT
All those Moths failures look a little sketchy, but I think I just got unlucky.

https://tbpl.mozilla.org/?tree=Try&rev=2832f51fae34
Comment 15 Andrew McCreight [:mccr8] 2012-04-30 09:29:18 PDT
Created attachment 619592 [details] [diff] [review]
unmark gray strongly held observers
Comment 16 Olli Pettay [:smaug] 2012-04-30 11:10:11 PDT
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
Comment 17 Andrew McCreight [:mccr8] 2012-04-30 12:02:57 PDT
Thanks for the reviews.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e2658794e06
Comment 18 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-02 21:11:12 PDT
https://hg.mozilla.org/mozilla-central/rev/0e2658794e06
Comment 19 Olli Pettay [:smaug] 2012-06-18 13:31:28 PDT
Should we think of landing this to FF14?
Comment 20 Benjamin Smedberg [:bsmedberg] 2012-06-18 14:27:14 PDT
I think this should ride the trains: it provides perf benefits but not correctness/stability benefits, right?
Comment 21 Andrew McCreight [:mccr8] 2012-06-18 14:31:26 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.