Adding cycle collection to something with a preserved wrapper is painful

RESOLVED FIXED in mozilla15

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: mccr8)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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

Comment 1

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

Updated

5 years ago
Assignee: nobody → continuation
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Comment 2

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

Comment 3

5 years ago
Created attachment 623303 [details] [diff] [review]
add WRAPPER_CACHE_{0,1} CC macros, use them
(Assignee)

Updated

5 years ago
Attachment #623303 - Flags: feedback?(bzbarsky)
Comment on attachment 623303 [details] [diff] [review]
add WRAPPER_CACHE_{0,1} CC macros, use them

I like!
Attachment #623303 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Updated

5 years ago
Attachment #623303 - Flags: review?(bugs)

Comment 5

5 years ago
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
Attachment #623303 - Flags: review?(bugs) → review+
(Assignee)

Comment 6

5 years ago
I moved the macro declarations to nsWrapperCache.h

https://hg.mozilla.org/integration/mozilla-inbound/rev/92469330d939
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/92469330d939
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.