737075 - unmark gray observers implemented in JS held by observer service

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

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

Attachment: hacky WIP

Comment 2

[Andrew McCreight [:mccr8]]

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

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

Attachment: less hacky patch

Comment 5

[Andrew McCreight [:mccr8]]

This appears to be leaky somehow.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: fix getter to avoid leak

Comment 7

[Andrew McCreight [:mccr8]]

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]

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

(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]]

Attachment: 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]]

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

Attachment: unmark gray strongly held observers

Comment 13

[Andrew McCreight [:mccr8]]

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

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

Attachment: unmark gray strongly held observers

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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

Thanks for the reviews.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e2658794e06

Comment 18

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/0e2658794e06

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Should we think of landing this to FF14?

Comment 20

[Benjamin Smedberg]

I think this should ride the trains: it provides perf benefits but not correctness/stability benefits, right?

Comment 21

[Andrew McCreight [:mccr8]]

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.