Closed Bug 874691 Opened 7 years ago Closed 7 years ago

Make CC participant tracing indirect

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: terrence, Assigned: jonco)

References

Details

Attachments

(1 file, 1 obsolete file)

This is the final piece required before we can start adding post-barriers to the browser.
Component: JavaScript Engine → XPCOM
I assume this is going to mostly be browser-side changes.

If you could post some kind of work-in-progress or plan, Jon, that Olli and I could look at it would be much appreciated.  I just want to make sure we're all agreed on an approach here.
Attached patch Patch (obsolete) — Splinter Review
Ok, here's the work so far.

The main changes are in xpcom/glue/nsCycleCollectionParticipant.h, where the TraceCallback function pointer typedef has been replaced with the TraceCallbacks struct which has overloaded virtual methods for tracing different types of JS GC thing.

Please let me know what you think!
Comment on attachment 752732 [details] [diff] [review]
Patch

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

Thanks, looks reasonable to me.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +6,5 @@
>  #ifndef nsCycleCollectionParticipant_h__
>  #define nsCycleCollectionParticipant_h__
>  
>  #include "nsCycleCollectionNoteChild.h"
> +#include "jspubtd.h"

Can you forward declare the JS types rather than #include?
(In reply to Andrew McCreight [:mccr8] from comment #3)

> Can you forward declare the JS types rather than #include?

This falls foul of the fact that jsid is declared differently in debug and opt builds (except where a macro is defined), and I didn't want to duplicate the logic here.
Attached patch Patch v2Splinter Review
Updated patch.

Try results are good, ignoring infrastructure craziness: https://tbpl.mozilla.org/?tree=Try&rev=4b40022e5c4c

mccr8, can you suggest suggest reviewers?
Attachment #752732 - Attachment is obsolete: true
Flags: needinfo?(continuation)
Comment on attachment 753264 [details] [diff] [review]
Patch v2

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

bz should review at least nsContentUtils.{h,cpp}, nsWrapperCache.h, and maybe make sure my suggestions about the const stuff in the nsXBLProto* files is reasonable.

smaug should review at least nsCycleCollectionParticipant.{h,cpp} and maybe skim over some of the other stuff.

It kind of looks like we should replace nsINode::Trace() with nsWrapperCache::TraceWrapper(), which will let us macro-ize things further, but that is an orthogonal issue, so I'll file a separate bug for it.

::: content/xbl/src/nsXBLProtoImpl.cpp
@@ +260,5 @@
>    return true;
>  }
>  
>  void
> +nsXBLProtoImpl::Trace(const TraceCallbacks& aCallbacks, void *aClosure) const

This is going to become unconst.

::: content/xbl/src/nsXBLProtoImplMember.h
@@ +78,5 @@
>    virtual nsresult CompileMember(nsIScriptContext* aContext,
>                                   const nsCString& aClassStr,
>                                   JS::Handle<JSObject*> aClassObject) = 0;
>  
> +  virtual void Trace(const TraceCallbacks& aCallbacks, void *aClosure) const = 0;

not const any more

::: content/xbl/src/nsXBLProtoImplMethod.cpp
@@ +224,5 @@
>    return NS_OK;
>  }
>  
>  void
> +nsXBLProtoImplMethod::Trace(const TraceCallbacks& aCallbacks, void *aClosure) const

This method isn't actually |const| any more, so it seems like you should drop that, which would let you remove the const cast below.  How badly does that break things?

::: content/xbl/src/nsXBLProtoImplProperty.cpp
@@ +299,5 @@
>    return rv;
>  }
>  
>  void
> +nsXBLProtoImplProperty::Trace(const TraceCallbacks& aCallbacks, void *aClosure) const

Likewise, this isn't |const| any more, so you should unconst this and remove the const casts.

::: dom/base/nsJSEnvironment.cpp
@@ +3582,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsJSArgArray)
> +  if (tmp->mArgv) {
> +    for (uint32_t i = 0; i < tmp->mArgc; ++i) {
> +      if (JSVAL_IS_GCTHING(tmp->mArgv[i]))

You don't need to test for |JSVAL_IS_GCTHING|, as |TRACE_JSVAL_MEMBER| already does that.

@@ -3583,5 @@
>  NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsJSArgArray)
> -  JS::Value *argv = tmp->mArgv;
> -  if (argv) {
> -    JS::Value *end;
> -    for (end = argv + tmp->mArgc; argv < end; ++argv) {

Ah yes I've seen this code before.  I have no idea what kind of microoptimization inspired somebody to walk through this array backwards.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +484,5 @@
>  #define NS_IMPL_CYCLE_COLLECTION_TRAVERSE_RAWPTR(_field)                       \
>    CycleCollectionNoteChild(cb, tmp->_field, #_field);
>  
>  #define NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS                       \
> +    {                                                                          \

nit: indent { and } by only 2 spaces

@@ +485,5 @@
>    CycleCollectionNoteChild(cb, tmp->_field, #_field);
>  
>  #define NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS                       \
> +    {                                                                          \
> +    TraceCallbackFunc noteJsChild(&nsScriptObjectTracer::NoteJSChild);      \

nit: noteJSChild, not noteJsChild
nit: line up the \ at the end
Attachment #753264 - Flags: review+
Flags: needinfo?(continuation)
Comment on attachment 753264 [details] [diff] [review]
Patch v2

See the top of comment 6.
Attachment #753264 - Flags: review?(bzbarsky)
Attachment #753264 - Flags: review?(bugs)
Blocks: 875409
Comment on attachment 753264 [details] [diff] [review]
Patch v2

>+struct TraceCallbackFunc : public TraceCallbacks
>+{
>+    typedef void (* Func)(void* p, const char* name, void* closure);
>+
>+    TraceCallbackFunc(Func cb) : callback(cb) {}
>+
>+    virtual void Trace(JS::Value* p, const char* name, void* closure) const;
>+    virtual void Trace(jsid* p, const char* name, void* closure) const;
>+    virtual void Trace(JSObject** p, const char* name, void* closure) const;
>+    virtual void Trace(JSString** p, const char* name, void* closure) const;
>+    virtual void Trace(JSScript** p, const char* name, void* closure) const;
>+
>+  private:
>+    Func callback;
mCallback please, 

And could you add some comments when to use TraceCallbackFunc
Attachment #753264 - Flags: review?(bugs) → review+
QA Contact: general
Comment on attachment 753264 [details] [diff] [review]
Patch v2

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +415,2 @@
>  {
> +    virtual void Trace(JS::Value *p, const char *name, void *closure) const {

I think we should annotate these as MOZ_OVERRIDE, if possible: should give you nicer errors at least when you change their signature tomorrow.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +69,5 @@
> +    typedef void (* Func)(void* p, const char* name, void* closure);
> +
> +    TraceCallbackFunc(Func cb) : callback(cb) {}
> +
> +    virtual void Trace(JS::Value* p, const char* name, void* closure) const;

MOZ_OVERRIDE would probably handy here too.
Attachment #753264 - Flags: feedback+
Comment on attachment 753264 [details] [diff] [review]
Patch v2

The wrapper cache and content utils changes are fine.

The XBL stuff needs to end up with non-const methods, imo, and without const_cast.

r=me on those bits
Attachment #753264 - Flags: review?(bzbarsky) → review+
Also, jonco, if you could add a comment to TraceCallbackFunc saying that it coerces untyped, non-moving tracing callbacks to TraceCallbacks that would be useful, as it wasn't obvious to me at a glance.
(In reply to Andrew McCreight [:mccr8] from comment #11)

I had second thoughts about this (and yes it's not obvious) so I changed the constructor to be explicit.
indentation looks wrong here:
   22.22 +      NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mArgv[i])
   22.23 +      }
https://hg.mozilla.org/mozilla-central/rev/76321fce71e7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.