Closed Bug 750570 Opened 12 years ago Closed 12 years ago

Suspect native cycle collected objects

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(5 files, 8 obsolete files)

4.21 KB, patch
smaug
: review+
Details | Diff | Splinter Review
20.91 KB, patch
smaug
: review+
Details | Diff | Splinter Review
17.03 KB, patch
glandium
: review+
Details | Diff | Splinter Review
21.50 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.03 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Native (ie non-nsISupports) cycle collected classes aren't added to the purple buffer.  If they happen to be the last reference to a garbage cycle, we get leaks.  This is not merely a theoretical problem.  In bug 750424, billm figured out that he was getting shutdown leaks with incremental GC because nsXULPrototypeNode wasn't getting added to the purple buffer.  In bug 737976, khuey's deCOM of nsINodeInfo was causing random leaks, perhaps because nsNodeInfo was no longer added to the purple buffer.  It is possible there are other lurking random leaks due to this.

The solution is to either add a new purple buffer for native CCed objects, or to add a new field to the existing purple buffer for the cycle collector participants.  The latter solution might be easier, but would require more stores to happen on a pretty hot path, so I don't know how bad that would be.

I think the first thing to do is to look at what native classes are out there.
Another option is to make non-ISupports classes inherit from something that has a virtual GetParticipant method.
Are there any advantages to a class not being nsISupports? Kyle, in bug 737976, why couldn't you just make nsNodeInfo inherit directly from nsISupports? Then it could be a normal cycle-collected class. I realize this is similar to having a GetParticipant method, but it just seems like it requires less extra machinery.
If we remove nsINodeInfo then nsNodeInfo doesn't need a vtable and everything would be a word smaller.  (Doing comment 1 would negate that advantage, of course).
Whiteboard: [MemShrink] → [MemShrink:P2]
(In reply to Bill McCloskey (:billm) from comment #2)
> Are there any advantages to a class not being nsISupports?

Non-virtual addref/release methods.
Bug 766717 is another instance of this problem.
Attached patch leaky WIP (obsolete) — Splinter Review
This is pretty ugly.

I get a lot of these:
###!!! ASSERTION: nsTimeout not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/amccreight/mz/cent4/dom/base/nsGlobalWindow.cpp, line 9858

I added those checks so maybe it isn't too bad, but it seems a little skeezy that we're failing a thread safety check in a class we want to interact with the purple buffer.  Maybe I'll just leave nsTimeout unchanged.

This patch also seems to leak a lot.
Attached patch now with less leaking (obsolete) — Splinter Review
This doesn't leak, at least with simple browsing.  I fixed the leaks by reverting my changes to nsTimeout, so they aren't being added to the purple buffer.
Attachment #638945 - Attachment is obsolete: true
Depends on: 774874
Attachment #638943 - Attachment is obsolete: true
My attempt to split this into a few smaller patches.
The type of this argument has to be changed anyways, so we might as well just delete it.
nsTimeout remains un-purplebuffered for the moment.
Attachment #639423 - Attachment is obsolete: true
I seem to remember that the leaks were more common on Windows.
The base run was okay, but the other ones were broken due to my bad attempt at a backout.  We need to figure out some way to statically assert when you declare something is a script holder, but then you don't put a Trace function into the vtable. At least I think that's what was going on.

Anywho, this time I actually ran the browser with the backouts before I pushed to try. I also did Windows this time, per Bill's suggestion.

without purple buffer power: https://tbpl.mozilla.org/?tree=Try&rev=6b66ab8be6e6
with purple buffer power: https://tbpl.mozilla.org/?tree=Try&rev=0ff00b0e380f

If this works, next I'm going to do some blind taste testing with people walking down Castro St., to see if they can tell the difference.
Well, Moth doesn't seem to leak on Windows any more with that patch backed out.
Attachment #650713 - Attachment is obsolete: true
Blocks: 782485
Blocks: 782735
Pushed to try, along with the patches for 782485 and 782735:
https://tbpl.mozilla.org/?tree=Try&rev=73aa90bca6bf
Comment on attachment 651491 [details] [diff] [review]
part 1: lift UnmarkIfPurple to the CC participant

This patch just moved UnmarkIfPurple up to the top level participant class, because we are making it so that all CC classes can be purple. It also removes some unused functions.
Attachment #651491 - Flags: review?(mh+mozilla)
Attachment #651491 - Flags: review?(bugs)
Attachment #651555 - Flags: review?(bugs)
Attachment #651021 - Flags: review?(bugs)
Attachment #651609 - Flags: review?(bugs)
Comment on attachment 651491 [details] [diff] [review]
part 1: lift UnmarkIfPurple to the CC participant

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

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +669,2 @@
>    {                                                                            \
> +    Downcast(static_cast<nsISupports *>(s))->UnmarkIfPurple();                 \

Side note: at some point, we should be able to use _class * instead of void *, here.

@@ -887,5 @@
>    const CCParticipantVTable<NS_CYCLE_COLLECTION_CLASSNAME(_class)>             \
>      ::Type _class::NS_CYCLE_COLLECTION_INNERNAME =                             \
>    { NS_IMPL_CYCLE_COLLECTION_NATIVE_VTABLE(NS_CYCLE_COLLECTION_CLASSNAME(_class)) };
>  
> -#define NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(_class)            \

Why remove this one? Because it would only be used in nsXPConnect, and isn't?
Attachment #651491 - Flags: review?(mh+mozilla) → review+
Attachment #651491 - Flags: review?(bugs) → review+
(In reply to Mike Hommey [:glandium] from comment #22)
> Why remove this one? Because it would only be used in nsXPConnect, and isn't?

I didn't actually remove it, I just moved it down a line. It is now the same as NS_IMPL_CYCLE_COLLECTION_CLASS. NS_IMPL_CYCLE_COLLECTION_CLASS is much more common, so I wanted to reduce the amount of indirection in the common case, so I defined NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS in terms of NS_IMPL_CYCLE_COLLECTION_CLASS rather than vice versa.
(In reply to Andrew McCreight [:mccr8] from comment #23)
> (In reply to Mike Hommey [:glandium] from comment #22)
> > Why remove this one? Because it would only be used in nsXPConnect, and isn't?
> 
> I didn't actually remove it, I just moved it down a line. It is now the same
> as NS_IMPL_CYCLE_COLLECTION_CLASS. NS_IMPL_CYCLE_COLLECTION_CLASS is much
> more common, so I wanted to reduce the amount of indirection in the common
> case, so I defined NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS in
> terms of NS_IMPL_CYCLE_COLLECTION_CLASS rather than vice versa.

Ah, I got confused by the simultaneous renaming, which actually bothers me. The reason why these macros are named this way is that they match the macros for NS_DECL_*. If you still want to rename, though, you should also change the VTABLE macro names in comments above the nsCycleCollectionParticipantType enum.
(In reply to Mike Hommey [:glandium] from comment #24)
> Ah, I got confused by the simultaneous renaming, which actually bothers me.
> The reason why these macros are named this way is that they match the macros
> for NS_DECL_*. If you still want to rename, though, you should also change
> the VTABLE macro names in comments above the
> nsCycleCollectionParticipantType enum.

The main IMPL macros should still match the main DECL macros, the change I made only made the VTABLE ones be out of sync, as I was viewing them as an internal detail. But you are right, it would be better to just do this:

#define NS_IMPL_CYCLE_COLLECTION_VTABLE(_class) \
NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_VTABLE(_class)

and maintain the parallel structure everywhere.  At least, I think this is what you mean?
If you mean the following, then yes

diff --git a/xpcom/glue/nsCycleCollectionParticipant.h b/xpcom/glue/nsCycleCollectionParticipant.h
--- a/xpcom/glue/nsCycleCollectionParticipant.h
+++ b/xpcom/glue/nsCycleCollectionParticipant.h
@@ -868,22 +868,22 @@ struct Skippable
   {                                                                            \
     &_class::TraverseImpl,                                                     \
     &_class::RootImpl,                                                         \
     &_class::UnlinkImpl,                                                       \
     &_class::UnrootImpl,                                                       \
+    &_class::UnmarkIfPurpleImpl,                                               \
     _class::isSkippable ? &Skippable<_class>::CanSkipImpl : NULL,              \
     _class::isSkippable ? &Skippable<_class>::CanSkipInCCImpl : NULL,          \
     _class::isSkippable ? &Skippable<_class>::CanSkipThisImpl : NULL           \
   }
  
 #define NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_VTABLE(_class)           \
   NS_IMPL_CYCLE_COLLECTION_NATIVE_VTABLE(_class),                              \
   { &_class::TraceImpl }
 
 #define NS_IMPL_CYCLE_COLLECTION_VTABLE(_class)                                \
-  NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_VTABLE(_class),                \
-  { &_class::UnmarkIfPurpleImpl }
+  NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_VTABLE(_class)
 
 #define NS_IMPL_CYCLE_COLLECTION_NATIVE_CLASS(_class)                          \
   const CCParticipantVTable<NS_CYCLE_COLLECTION_CLASSNAME(_class)>             \
     ::Type _class::NS_CYCLE_COLLECTION_INNERNAME =                             \
   { NS_IMPL_CYCLE_COLLECTION_NATIVE_VTABLE(NS_CYCLE_COLLECTION_CLASSNAME(_class)) };


Although NS_IMPL_CYCLE_COLLECTION_CLASS could be changed, too:
#define NS_IMPL_CYCLE_COLLECTION_CLASS(_class) \
  NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(_class)
Attachment #651491 - Attachment is obsolete: true
Comment on attachment 652496 [details] [diff] [review]
part 1: lift UnmarkIfPurple to the CC participant

How's this?  I only changed the stuff starting from:
// VTables for the cycle collector participant implementations.

I like having NS_IMPL_CYCLE_COLLECTION_CLASS be defined in terms of NS_IMPL_CYCLE_COLLECTION_VTABLE, because then any changes to its _VTABLE will be automatically reflected.
Attachment #652496 - Flags: review?(mh+mozilla)
Comment on attachment 652496 [details] [diff] [review]
part 1: lift UnmarkIfPurple to the CC participant

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

That's what i wrote in comment 26 :)
Attachment #652496 - Flags: review?(mh+mozilla) → review+
Yeah, I realized that after I uploaded it... Thanks.
Unbitrotting from changes to part 1.
Attachment #651609 - Attachment is obsolete: true
Attachment #651609 - Flags: review?(bugs)
Attachment #652500 - Flags: review?(bugs)
Comment on attachment 651555 [details] [diff] [review]
part 2: add purple buffer support for non-nsISupports classes






> struct nsPurpleBufferEntry {
>   union {
>-    nsISupports *mObject;                 // when low bit unset
>+    void *mObject;                        // when low bit unset
>     nsPurpleBufferEntry *mNextInFreeList; // when low bit set
>   };
>   // When an object is in the purple buffer, it replaces its reference
>   // count with a (tagged) pointer to this entry, so we store the
>   // reference count for it.
>   nsrefcnt mRefCnt;
>+  nsCycleCollectionParticipant *mParticipant; // NULL for nsISupports
> };
So, this increases purple buffer's memory usage. It shouldn't be usually horribly bad though.
But, since we get mParticipant, we should utilize it fully... is there any way we could pass the
participant also in nsISupports case? It would be great if we wouldn't have to QI nsCycleCollectionISupports
when running CC. Maybe not any fast way.

Have you profiled addref/release? Do your patches slow them down? (addref/release are already slower than we'd like them to be.)

code looks good, r=me for that, but please do some profiling, just in case...
Attachment #651555 - Flags: review?(bugs) → review+
Attachment #651021 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #32)
> So, this increases purple buffer's memory usage. It shouldn't be usually
> horribly bad though.

Yeah, we fire off a CC after 5 seconds when there are 100 or so things in the purple buffer, so it never gets that big.

> But, since we get mParticipant, we should utilize it fully... is there any
> way we could pass the
> participant also in nsISupports case? It would be great if we wouldn't have
> to QI nsCycleCollectionISupports
> when running CC. Maybe not any fast way.

I'm not sure.  It would require redefining AddRef/Release for every class that has its own participant. I'm not sure if that is done currently. I don't think QI takes much time in the CC.

> Have you profiled addref/release? Do your patches slow them down?
> (addref/release are already slower than we'd like them to be.)
> 
> code looks good, r=me for that, but please do some profiling, just in case...

How should I do that?  at a micro level with something like Shark, or look at the broader setting with Talos?  Both?

For nsISupports classes, this is only going to add an additional NULL store (with good locality: it is right next to another store we're already doing) in the slow case when we're putting things in the purple buffer, so I think it shouldn't be too bad, but you are right, I should try to measure it.
Addref/release show up in the micro-benchmarks.
Comment on attachment 652500 [details] [diff] [review]
part 4: switch most native CC classes over to using the purple buffer

> 
>-NS_IMPL_CYCLE_COLLECTION_NATIVE_CLASS(nsTimeout)
>+NS_IMPL_CYCLE_COLLECTION_NON_PURPLE_NATIVE_CLASS(nsTimeout)
> NS_IMPL_CYCLE_COLLECTION_UNLINK_NATIVE_0(nsTimeout)
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_BEGIN(nsTimeout)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mWindow,
>                                                        nsIScriptGlobalObject)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mPrincipal)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mScriptHandler)
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(nsTimeout, AddRef)
>diff --git a/dom/base/nsGlobalWindow.h b/dom/base/nsGlobalWindow.h
>--- a/dom/base/nsGlobalWindow.h
>+++ b/dom/base/nsGlobalWindow.h
>@@ -133,17 +133,17 @@ NS_CreateJSTimeoutHandler(nsGlobalWindow
>  * timeout.  Holds a strong reference to an nsIScriptTimeoutHandler, which
>  * abstracts the language specific cruft.
>  */
> struct nsTimeout : PRCList
> {
>   nsTimeout();
>   ~nsTimeout();
> 
>-  NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(nsTimeout)
>+  NS_DECL_CYCLE_COLLECTION_NON_PURPLE_NATIVE_CLASS(nsTimeout)
> 
>   nsrefcnt Release();
>   nsrefcnt AddRef();
> 
>   nsTimeout* Next() {
>     // Note: might not actually return an nsTimeout.  Use IsTimeout to check.
>     return static_cast<nsTimeout*>(PR_NEXT_LINK(this));
>   }



> #endif // nsTransactionItem_h__
>diff --git a/xpcom/glue/nsCycleCollectionParticipant.h b/xpcom/glue/nsCycleCollectionParticipant.h
>--- a/xpcom/glue/nsCycleCollectionParticipant.h
>+++ b/xpcom/glue/nsCycleCollectionParticipant.h
>@@ -854,16 +854,19 @@ struct Skippable
> 
> // Cycle collector participant implementations.
> 
> #define NS_IMPL_CYCLE_COLLECTION_NATIVE_CLASS(_class)                          \
>   const CCParticipantVTable<NS_CYCLE_COLLECTION_CLASSNAME(_class)>             \
>     ::Type _class::NS_CYCLE_COLLECTION_INNERNAME =                             \
>   { NS_IMPL_CYCLE_COLLECTION_NATIVE_VTABLE(NS_CYCLE_COLLECTION_CLASSNAME(_class)) };
> 
>+#define NS_IMPL_CYCLE_COLLECTION_NON_PURPLE_NATIVE_CLASS(_class)               \
>+  NS_IMPL_CYCLE_COLLECTION_NATIVE_CLASS(_class)
>+


>+#define NS_DECL_CYCLE_COLLECTION_NON_PURPLE_NATIVE_CLASS(_class)               \
>+  class NS_CYCLE_COLLECTION_INNERCLASS                                         \
>+   : public nsCycleCollectionParticipant                                       \
>+  {                                                                            \
>+     NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS_BODY(_class)                        \
>+     NS_DECL_CYCLE_COLLECTION_STUB_UNMARK_IF_PURPLE(_class)                    \
>+  };                                                                           \
>+  NS_CYCLE_COLLECTION_PARTICIPANT_INSTANCE
>+

_NON_PURPLE_ is really odd. It assume it won't be clear to API users. needs some documentation.
Attachment #652500 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #35)
> _NON_PURPLE_ is really odd. It assume it won't be clear to API users. needs
> some documentation.

Good point. Hopefully it is just a weird legacy thing that only nsTimeout will need, but I should still describe it.
No longer depends on: 774874
I'll land this folded into part 4, but I'm leaving it separate here to make it easier to review.

I renamed the NON_PURPLE macros to LEGACY, and added some description (perhaps in the wrong place in the file) what these various kinds of cycle collected classes are for.
Attachment #654758 - Flags: review?(bugs)
Comment on attachment 652500 [details] [diff] [review]
part 4: switch most native CC classes over to using the purple buffer

patch 4 is unchanged from before, but will be modified with the stuff in 4b when I land it.
Attachment #652500 - Flags: review- → review?(bugs)
Attachment #654758 - Flags: review?(bugs) → review+
Attachment #652500 - Flags: review?(bugs) → review+
You need to log in before you can comment on or make changes to this bug.