754151 - Adding cycle collection to something with a preserved wrapper is painful

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Just now, I changed a class from doing CC on a single nsCOMPtr member to having the nsCOMPtr member and an optional preserved wrapper.

Before the change, in addition to the two NS_DECL lines in the header, the class had this in the C++:

  NS_IMPL_CYCLE_COLLECTION_1(nsComputedDOMStyle, mContent)

(ignoring the skippability goop).

After the change, still ignoring skippability, I need to have this:

  NS_IMPL_CYCLE_COLLECTION_CLASS(nsComputedDOMStyle)

  NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsComputedDOMStyle)
    NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
  NS_IMPL_CYCLE_COLLECTION_TRACE_END

  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsComputedDOMStyle)
    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mContent)
    NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
  NS_IMPL_CYCLE_COLLECTION_UNLINK_END

  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsComputedDOMStyle)
    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mContent)
    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

this is complete boilerplate that I'd love to replace with:

  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(nsComputedDOMStyle, mContent)

or something.  Furthermore, it's boilerplate that all new-binding classes will need, since they all have wrappercaches that can preserve wrappers.  So having a simple way to implement it would be really nice.

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Sounds reasonable.  We could reduce the boilerplate for a few other classes, too:

NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_BOILERPLATE_COLLECTION_0: nsDOMFileList, nsDOMTokenList, nsChildContentList, WebGLExtension

NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_BOILERPLATE_COLLECTION_1: nsPaintRequestList, nsClientRectList

Comment 2

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: rough taxonomy of tracing CC classes

I looked at every script holding CC class, and drew up this crude categorization.  I didn't really write up the INHERITED ones because they are pretty non-generic.  In addition to the full blown _0 and _1 sort of things bz suggested, it looks like the generic TRACE could be macroized.

That's this sequence:
 NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(classname)
   NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
 NS_IMPL_CYCLE_COLLECTION_TRACE_END

I'm not sure if that's all that useful.

I also noticed a number of TRACE_JS_CALLBACKs that could be cleaned up, plus we should add a TRACE_JS_VAL_CALLBACK as that is used all over the place.  I'll file a separate bug for those.

Those are the only improvements that leaped out at me.

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: add WRAPPER_CACHE_{0,1} CC macros, use them

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 623303 [details] [diff] [review]
add WRAPPER_CACHE_{0,1} CC macros, use them

I like!

Comment 5

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

Comment on attachment 623303 [details] [diff] [review]
add WRAPPER_CACHE_{0,1} CC macros, use them

># HG changeset patch
># User Andrew McCreight <[e-mail]>
># Date 1336771631 25200
># Node ID ccb4736ed28a2dbd5cb71f2c52c6b9764a441bfd
># Parent  40b7177ea13a1cb9cff71adf2f4b24e763c1f434
>Bug 754151 - add macros for basic wrappercached cycle collected classes. r=smaug
>
>diff --git a/content/base/public/nsINode.h b/content/base/public/nsINode.h
>--- a/content/base/public/nsINode.h
>+++ b/content/base/public/nsINode.h
>@@ -1670,10 +1670,36 @@ NS_DEFINE_STATIC_IID_ACCESSOR(nsINode, N
> 
> 
> #define NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER \
>   nsContentUtils::TraceWrapper(tmp, aCallback, aClosure);
> 
> #define NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER \
>   nsContentUtils::ReleaseWrapper(s, tmp);
> 
>+#define NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(_class) \
>+  NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(_class)              \
>+    NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER        \
>+  NS_IMPL_CYCLE_COLLECTION_TRACE_END
>+
>+#define NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(_class) \
>+  NS_IMPL_CYCLE_COLLECTION_CLASS(_class)                \
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(_class)         \
>+    NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER   \
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_END                   \
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(_class)       \
>+    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS    \
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END                 \
>+  NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(_class)
>+
>+#define NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(_class, _field) \
>+  NS_IMPL_CYCLE_COLLECTION_CLASS(_class)                        \
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(_class)                 \
>+    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(_field)            \
>+    NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER           \
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_END                           \
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(_class)               \
>+    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(_field)          \
>+    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS            \
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END                         \
>+  NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(_class)
> 
Strange place for this stuff.
Could you move this to nsWrapperCache.h ?
Also NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER and
NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER

Comment 6

[Andrew McCreight (out of office until 8/21) [:mccr8]]

I moved the macro declarations to nsWrapperCache.h

https://hg.mozilla.org/integration/mozilla-inbound/rev/92469330d939

Comment 7

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/92469330d939