Last Comment Bug 754151 - Adding cycle collection to something with a preserved wrapper is painful
: Adding cycle collection to something with a preserved wrapper is painful
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-10 18:50 PDT by Boris Zbarsky [:bz]
Modified: 2012-05-14 14:15 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rough taxonomy of tracing CC classes (3.87 KB, text/plain)
2012-05-11 14:00 PDT, Andrew McCreight [:mccr8]
no flags Details
add WRAPPER_CACHE_{0,1} CC macros, use them (9.99 KB, patch)
2012-05-11 14:33 PDT, Andrew McCreight [:mccr8]
bugs: review+
bzbarsky: feedback+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-05-10 18:50:57 PDT
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 [:mccr8] 2012-05-10 19:24:58 PDT
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 [:mccr8] 2012-05-11 14:00:10 PDT
Created attachment 623292 [details]
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 [:mccr8] 2012-05-11 14:33:08 PDT
Created attachment 623303 [details] [diff] [review]
add WRAPPER_CACHE_{0,1} CC macros, use them
Comment 4 Boris Zbarsky [:bz] 2012-05-11 14:41:51 PDT
Comment on attachment 623303 [details] [diff] [review]
add WRAPPER_CACHE_{0,1} CC macros, use them

I like!
Comment 5 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-05-14 05:36:58 PDT
Comment on attachment 623303 [details] [diff] [review]
add WRAPPER_CACHE_{0,1} CC macros, use them

># HG changeset patch
># User Andrew McCreight <amccreight@mozilla.com>
># 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 [:mccr8] 2012-05-14 08:28:48 PDT
I moved the macro declarations to nsWrapperCache.h

https://hg.mozilla.org/integration/mozilla-inbound/rev/92469330d939
Comment 7 Matt Brubeck (:mbrubeck) 2012-05-14 14:15:16 PDT
https://hg.mozilla.org/mozilla-central/rev/92469330d939

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