unmark gray observers implemented in JS held by observer service

RESOLVED FIXED in mozilla15

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P2])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 607283 [details] [diff] [review]
hacky WIP
(Assignee)

Updated

5 years ago
Depends on: 738700
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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)

Comment 4

5 years ago
Created attachment 609356 [details] [diff] [review]
less hacky patch
Assignee: nobody → continuation
Attachment #607283 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
This appears to be leaky somehow.
(Assignee)

Comment 6

5 years ago
Created attachment 610281 [details] [diff] [review]
fix getter to avoid leak
Attachment #609356 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [Snappy]
(Assignee)

Comment 7

5 years ago
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-
(Assignee)

Comment 9

5 years ago
(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!
(Assignee)

Updated

5 years ago
Whiteboard: [Snappy] → [Snappy:P2]
(Assignee)

Comment 10

5 years ago
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.
Attachment #610281 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Though maybe that's just a bug with nsObserverService.h, as it seems silly to export a header that you can't use.
(Assignee)

Comment 12

5 years ago
Created attachment 618488 [details] [diff] [review]
unmark gray strongly held observers
Attachment #618468 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
All those Moths failures look a little sketchy, but I think I just got unlucky.

https://tbpl.mozilla.org/?tree=Try&rev=2832f51fae34
(Assignee)

Comment 15

5 years ago
Created attachment 619592 [details] [diff] [review]
unmark gray strongly held observers
Attachment #618488 - Attachment is obsolete: true
Attachment #619592 - Flags: review?(bugs)
Attachment #619592 - Flags: review?(benjamin)
Attachment #619592 - Flags: review?(benjamin) → review+
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+
(Assignee)

Comment 17

5 years ago
Thanks for the reviews.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e2658794e06
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/0e2658794e06
Status: NEW → RESOLVED
Last Resolved: 5 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?
(Assignee)

Comment 21

5 years ago
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.