715761 - Refactor XPConnect GC tracing

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think this code could use an overhaul.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: big comment explaining XPC data structures

I always found XPConnect to be pretty intimidating. Hopefully this comment will help someone.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: refactor automarkers to use templates

This is a pretty simple refactoring to get rid of the macros. Also, the code previously seemed to assume that AutoMarkingPtrs could be destructed in non-LIFO order. I don't see how this could happen, so I got rid of it.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: change how mark bits are stored

This code changes how mark bits are stored for NativeInterfaces and NativeSets. Previously, setting the mark bit would affect the information they stored. Now the mark bit is totally independent. I think the size of the data structures should be the same.

Additionally, the tracing functions for WrappedNatives and WrappedNativeProtos would avoid setting the mark bits for their NativeSets because of the problem above (setting the mark bit would mess up the interface count; the big comment in TraceForValidWrapper explains this issue in greater depth). Now that that's fixed, we can do the marking during GC tracing.

Note: the goal here is to avoid the separate marking stage that we have to do after all other GC marking is complete. That's what these later patches basically do...

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: split apart TraceJS into TraceSelf and TraceInside

The basic organization of XPConnect is as follows:

      C++                          JS
  XPCWrappedNative      <--> mFlattenedJSObject
  XPCWrappedNativeProto <--> mJSProtoObject
  XPCWrappedNativeScope <--> mGlobalJSObject

The goal of this patch is to have two GC tracing methods for each of the C++ objects above. The TraceSelf method just traces the corresponding JS object (i.e., mFlattenedJSObject for XPCWrappedNative). The TraceInside method traces through the fields of the given C++ object (for example, the WrappedNative will trace through its proto or its scope).

The ideal organization would be for each JS object to have a trace hook in its JSClass like the following:
  void trace_hook(JSTracer *trc, JSObject *obj) {
    NativeClass *x = GetNativeFromJSObj(obj);
    x->TraceInside(trc);
  }

...and its TraceInside implementation would look like the following:
  void TraceInside(JSTracer *trc) {
    field1->TraceSelf(trc);
    field2->TraceSelf(trc);
  }

This is how a lot of tracing code in the JS engine works.

This patch splits the TraceJS methods into TraceSelf/TraceInside, but leaves the semantics the same. Later patches actually change the structure of tracing.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: trace scope from proto

This code changes some of the tracing code to use TraceSelf/TraceInside rather than TraceJS.

It also moves around where scopes are traced. Now the TraceInside methods for WrappedNatives and WrappedNativeProtos trace the scope. Previously, scope tracing always happened separately.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: eliminate special marking after GC marking

This patch has three pieces. First, it passes around a flag to each GC callback saying whether it's a compartment GC. This is needed for the second part.

Second, it eliminates the special marking of native sets, native interfaces, and ScriptableShared data after normal GC marking. We can do this now because the normal JS GC marking sets the mark bits itself.

However, we have to be careful. The third piece makes sure that we only sweep native sets, native interfaces, and scriptable shared stuff on a full (non-compartment) GC. During a compartment GC, the mark bits will only be set for the native sets reachable from a given compartment, and we have no way to sweep only a single compartment's native sets.

However, we already have places in the GC where a full GC is needed to clean stuff up, so this isn't so bad.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rename XPCWrappedNativeScope::TraceJS

Simple renaming patch.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch to change AutoMarkingPtrs to use TraceSelf

Now that we've done away with the separate mark phase, we can change the AutoMarkingPtrs to use the TraceSelf of most of these objects. For simple objects like XPCNativeInterface, we just call Mark.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 586300 [details] [diff] [review]
big comment explaining XPC data structures

>+ In some cases, there is an additional C++ object involved of type XPCWrappedNative.

Maybe say "in the default case" or "in general" here to imply that this is the way most of XPConnect expects things to be?

The XPCWrappedNative
>+ * holds pointers to the C++ object and the flat JS object. As an optimization,
>+ * some C++ objects don't have XPCWrappedNatives, although they still have a
>+ * corresponding flat JS object. These are called "slim wrappers": all the
>+ * wrapping information is stored in extra fields of the C++ object and the JS
>+ * object.

Perhaps mention that slim wrappers are only used for the DOM? And that we bail out of the optimization case when we need to by "morphing" slim wrappers into XPCWrappedNatives?

>+ * All XPCWrappedNative objects belong to an XPCWrappedNativeScope. These scopes
>+ * are essentially in 1:1 correspondence with JS global objects. (The exception
>+ * is that an XPCWrappedNativeScope need not exist for globals where no wrapping
>+ * has taken place.)

Oh, interesting - when does this happen?

The XPCWrappedNativeScope has a pointer to the JS global
>+ * object. The parent of a flat JS object is the global JS object corresponding
>+ * to the wrapper's XPCWrappedNativeScope.

This is incorrect. For objects with classinfo, we call the PreCreate hook in XPCWrappedNative::GetNewOrUsed to determine our parent. For the DOM, XUL nodes are parented to their XUL parent, all other nodes are parented to their document,  and documents are parented to the global. The dirt is here: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#7339

>+ * Some C++ objects (notably DOM objects) have information associated with them
>+ * that lists the interfaces implemented by these objects. A C++ object exposes
>+ * this information by implementing nsIClassInfo.

It's worth noting that this isn't usually done manually via inheritance. It's done via macros in the QI implementation of the class.

> If a C++ object implements
>+ * nsIClassInfo, then JS code can call its methods without needing to use
>+ * QueryInterface first. Typically, all instances of a C++ class share the same
>+ * nsIClassInfo instance. (That is, obj->QueryInterface(nsIClassInfo) returns
>+ * the same result for every obj of a given class.)
>+ *
>+ * XPConnect tracks nsIClassInfo information in an XPCWrappedNativeProto object.
>+ * A given XPCWrappedNativeScope will have one XPCWrappedNativeProto for each
>+ * nsIClassInfo instance being used. The XPCWrappedNativeProto has an associated
>+ * JS object, which is used as the prototype of all flat JS objects created for
>+ * C++ objects with the given nsIClassInfo.
>+ *
>+ * Each XPCWrappedNativeProto has a pointer to its XPCWrappedNativeScope. If an
>+ * XPCWrappedNative wraps a C++ object with class info, then it points to its
>+ * XPCWrappedNativeProto. Otherwise it points to its XPCWrappedNativeScope. (The
>+ * pointers are smooshed together in a tagged union.) Either way it can reach
>+ * its scope.
>+ *
>+ * In the case of slim wrappers (where there is no XPCWrappedNative), the flat
>+ * JS object has a pointer to the XPCWrappedNativeProto stored in a reserved
>+ * slot.
>+ *
>+ * An XPCWrappedNativeProto keeps track of the set of interfaces implemented by
>+ * the C++ object in an XPCNativeSet. (The list of interfaces is obtained by
>+ * calling a method on the nsIClassInfo.) The list is stored in an
>+ * XPCNativeSet.

A bit of redundancy here with "XPCNativeSet".

>    An XPCNativeSet is a collection of XPCNativeInterfaces. Each
>+ * interface stores the list of members, which can be methods, constants,
>+ * getters, or setters.
>+ *
>+ * An XPCWrappedNative also points to an XPCNativeSet. If the C++ object it is
>+ * wrapping has class info, this is the same as the XPCWrappedNativeProto's
>+ * set.

Not entirely. An XPCWrappedNative can have a set different than its XPCWrappedNativeProto, in which case it is said to have a "mutated" set.

>    If there is no class info, then this set starts out holding only the
>+ * nsISupports interface. As the JS code QI's more interfaces, the set grows.

Might be good to mention that the additional QI-ed native pointers are cached in XPCWrappedNativeTearOffs, so that the pointer doesn't have to be be QI-ed for each method call.

>+ * Besides having class info, a C++ object may be "scriptable" (i.e., implement
>+ * nsIXPCScriptable). This allows it to implement a more JS-friendly interface,

It's really more about implementing the DOM, because the DOM is too funky to be fully described by XPIDL. This is how we get element array functionality with [] (like plugin arrays), how the document.location/document.location.href stuff works, and how parenting (described above) works.

>+ * besides just exposing methods and constants. An nsIXPCScriptable instance has
>+ * hooks that correspond to all the normal JSClass hooks. Each nsIXPCScriptable
>+ * instance is mirrored by an XPCNativeScriptableInfo in XPConnect. These can
>+ * have pointers from XPCWrappedNativeProto and XPCWrappedNative (since C++
>+ * objects can have scriptable info without having class info).
>+ *
>+ * Most data in an XPCNativeScriptableInfo is shared between instances. The
>+ * shared data is stored in an XPCNativeScriptableShared object. This type is
>+ * important because it holds the JSClass of the flat JS objects with the given
>+ * scriptable info.
>+ */
>+

Awesome work! r=bholley with the above comments addressed.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 586304 [details] [diff] [review]
refactor automarkers to use templates

> Also, the code
> previously seemed to assume that AutoMarkingPtrs could be destructed in
> non-LIFO order. I don't see how this could happen, so I got rid of it.

Well, it actually could, because we allow initialization to happen at a different time than construction via the Init() method. So someone could declare two AutoMarkingPtrs and initialize them in an order opposite to the one with which they were declared. Since destructor ordering is based on declaration ordering, this would be a problem.

I think we should honestly just kill the Init() method and disallow that feature. It's only used once, as a micro-optimization in XPCConvert, where we want the pointer to be scoped for the method, but we might not need it, so we avoid giving it the ccx (and thus having it twiddle the TLS pointers) unless we pass a conditional. I very much doubt that this optimization matters enough to justify the complexity here, so I say kill it and require a ccx to declare an AutoMarkingPtr.

> void
> XPCPerThreadData::Cleanup()
> {
>-    while (mAutoRoots)
>-        mAutoRoots->Unlink();
>+    MOZ_ASSERT(!mAutoRoots);

I've managed to convince myself that we only call this during thread teardown and nsXPConnect teardown, so this should be ok. It took some digging though. ;-)

>     virtual void TraceJS(JSTracer* trc) = 0;
>     virtual void MarkAfterJSFinalize() = 0;

Can we mark these |protected| to make sure nobody accidentally calls them when they mean to call TraceJSAll or MarkAfterJSFinalizeAll?


>+private:
>+    AutoMarkingPtr** mRoot;

This variable just serves to indicate whether we've inserted ourselves or not. But since (per above) we're tying initialization to construction, we can always assume that we have, and thus eliminate this member.

>+template<class T>
>+class TypedAutoMarkingPtr : public AutoMarkingPtr
...
>+private:
>+    T *mPtr;

I know XPConnect is very inconsistent about this, but I think T* would be more consistent here.

>+template<class T>
>+class ArrayAutoMarkingPtr : public AutoMarkingPtr
...
>+private:
>+    T **mPtr;

And here.

r- because I want to look at the ccx/Init stuff. But looks great in general!

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 586304 [details] [diff] [review]
refactor automarkers to use templates

Err, + => -.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 586306 [details] [diff] [review]
change how mark bits are stored

Oh good. I hated these!


>     nsCOMPtr<nsIInterfaceInfo> mInfo;
>     jsid                       mName;
>-    PRUint16          mMemberCount;
>-    XPCNativeMember   mMembers[1]; // always last - object sized for array
>+    PRUint16                   mMemberCount;
>+    PRUint16                   mMarked;
>+    XPCNativeMember            mMembers[1]; // always last - object sized for array
> };

I still don't really like depending on malloc alignment for this to be a wash memory-wise. Can we just make mMemberCount |:15| and add a |bool mMarked : 1|?


>-    XPCNativeSet()  {MOZ_COUNT_CTOR(XPCNativeSet);}
>+    XPCNativeSet() : mMarked(false) {MOZ_COUNT_CTOR(XPCNativeSet);}

Shouldn't this also zero out mInterfaceCount and mMemberCount?
 

>-        if (mScriptableInfo && JS_IsGCMarkingTracer(trc))
>-            mScriptableInfo->Mark();
>+        if (JS_IsGCMarkingTracer(trc)) {
>+            mSet->Mark();
>+            if (mScriptableInfo)
>+                mScriptableInfo->Mark();
>+        }

>         if (HasProto()) GetProto()->TraceJS(trc);

Can you break that into two lines while you're there?

>         JSObject* wrapper = GetWrapperPreserveColor();
>         if (wrapper)
>             JS_CALL_OBJECT_TRACER(trc, wrapper, "XPCWrappedNative::mWrapper");
>         if (mScriptableInfo &&
>             (mScriptableInfo->GetJSClass()->flags & JSCLASS_XPCONNECT_GLOBAL))
>             GetScope()->TraceDOMPrototypes(trc);
>     }


r+ with those comments addressed.

I'm flagging mrbkap for feedback here for him to do a high-level sign-off on two things:
1 - Switching to bitfields
2 - Marking mSet in XPCWrappedNative{,Proto}::TraceJS

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Bobby Holley (:bholley) from comment #12)
> I still don't really like depending on malloc alignment for this to be a
> wash memory-wise. Can we just make mMemberCount |:15| and add a |bool
> mMarked : 1|?

AIUI,

uint16_t a : 15;
bool     b :  1;

will not pack correctly with MSVC.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: refactor automarkers to use templates, v2

Good catch on the Init thing. I fixed it.

The mRoot field is needed in the destructor to unlink the automarker.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 586316 [details] [diff] [review]
patch to change AutoMarkingPtrs to use TraceSelf

Oops, I marked the wrong patch as obsolete.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Ms2ger from comment #13)
> (In reply to Bobby Holley (:bholley) from comment #12)
> > I still don't really like depending on malloc alignment for this to be a
> > wash memory-wise. Can we just make mMemberCount |:15| and add a |bool
> > mMarked : 1|?
> 
> AIUI,
> 
> uint16_t a : 15;
> bool     b :  1;
> 
> will not pack correctly with MSVC.

I don't think this depends on malloc. It's just the way that the C compiler packs structs. However, I can change it to use a bitfield.

I can also change the bool field to be PRUint16 as well. Will MSVC pack it properly then?

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Bill McCloskey (:billm) from comment #16)
> (In reply to Ms2ger from comment #13)
> > (In reply to Bobby Holley (:bholley) from comment #12)
> > > I still don't really like depending on malloc alignment for this to be a
> > > wash memory-wise. Can we just make mMemberCount |:15| and add a |bool
> > > mMarked : 1|?
> > 
> > AIUI,
> > 
> > uint16_t a : 15;
> > bool     b :  1;
> > 
> > will not pack correctly with MSVC.
> 
> I don't think this depends on malloc. It's just the way that the C compiler
> packs structs. However, I can change it to use a bitfield.
> 
> I can also change the bool field to be PRUint16 as well. Will MSVC pack it
> properly then?

Yeah, I think so. khuey?

Comment 18

[Bobby Holley (:bholley)]

(In reply to Ms2ger from comment #17)
> Yeah, I think so. khuey?

khuey said on IRC that this should work.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 586306 [details] [diff] [review]
change how mark bits are stored

>--- a/js/xpconnect/src/XPCInlines.h
>+++ b/js/xpconnect/src/XPCInlines.h
> JSObject* XPCWrappedNativeTearOff::GetJSObject() const
> {
>-    return mJSObject;
>+    return (JSObject *)((jsword)mJSObject & ~1);

|reinterpret_cast<JSObject*>(reinterpret_cast<intptr_t>(mJSObject) & ~1)|, please. (Ref: bug 714728)

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 586314 [details] [diff] [review]
eliminate special marking after GC marking

>--- a/js/xpconnect/src/XPCJSRuntime.cpp
>+++ b/js/xpconnect/src/XPCJSRuntime.cpp
>+NativeInterfaceUnmarker(JSDHashTable *table, JSDHashEntryHdr *hdr,
>+                       uint32_t number, void *arg)
>+NativeSetUnmarker(JSDHashTable *table, JSDHashEntryHdr *hdr,
>+               uint32_t number, void *arg)
>+JSClassUnmarker(JSDHashTable *table, JSDHashEntryHdr *hdr,
>+               uint32_t number, void *arg)

Indentation. (Bonus points for static_casts)

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 586493 [details] [diff] [review]
refactor automarkers to use templates, v2

Ah crap. I didn't notice that this would involve initializing a full ccx out of the lazy ccx in all cases in XPCConvert::NativeInterface2JSObject. It might be too hot (though I don't know for sure).

I think the best solution is to make the superclass take an XPCPerThreadData* in its constructor, and have the subclasses provide two constructors versions - one which takes a lccx, and the other which takes an XPCPerThreadData* directly. That way, the caller in NativeInterface2JSObject can grab its own via XPCThreadData::GetData(JSContext*). Or it could just take a JSContext and call that method internally. I don't really have an opinion there.

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 586493 [details] [diff] [review]
refactor automarkers to use templates, v2

Bill pointed out on IRC that we'll end up constructing a ccx in almost all cases anyway. The particular ones I was worried about (mostly the slim wrapper case, which should be very hot) were actually above the place where we're declaring it here, so I think it should be fine.

Assuming interdiff isn't lying, looks great! r=bholley

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 586309 [details] [diff] [review]
split apart TraceJS into TraceSelf and TraceInside

This is getting exciting!

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 586311 [details] [diff] [review]
trace scope from proto


> TraceForValidWrapper(JSTracer *trc, XPCWrappedNative* wrapper)
> {
>     // NOTE: It might be nice to also do the wrapper->Mark() call here too
>     // when we are called during the marking phase of JS GC to mark the
>     // wrapper's and wrapper's proto's interface sets.
>     //
>     // We currently do that in the GC callback code. The reason we don't do that
>     // here is because the bits used in that marking do unpleasant things to the
>@@ -665,36 +659,34 @@ TraceForValidWrapper(JSTracer *trc, XPCW
>     // danger is that a live wrapper might not be in a wrapper map and thus
>     // won't be fully marked in the GC callback. This can happen if there is
>     // a security exception during wrapper creation or if during wrapper
>     // creation it is determined that the wrapper is not needed. In those cases
>     // the wrapper can never actually be used from JS code - so resources like
>     // the interface set will never be accessed. But the JS engine will still
>     // need to use the JSClass. So, some marking is required for protection.
> 
>-    wrapper->TraceJS(trc);
>-
>-    TraceScopeJSObjects(trc, wrapper->GetScope());
>+    wrapper->TraceInside(trc);
> }

This change makes TraceForValidWrapper(trc, wrapper) equivalent to wrapper->TraceInside(trc). Let's just kill TraceForValidWrapper entirely, and replace the comment here with a big comment somewhere explaining the whole TraceInside/TraceSelf setup (and making clear that calling JS_CallTracer on a JSObject* guarantees that its trace hook is called, which I did not know until you told me).

Also, it looks like the caller (MarkWrappedNative) is called only by XPC_WN_NoHelper_Trace. So while we're at it, let's fold all this into XPC_WN_NoHelper_Trace.

> 
> static void
> MarkWrappedNative(JSTracer *trc, JSObject *obj)
> {
>     JSObject *obj2;
> 
>     // Pass null for the first JSContext* parameter  to skip any security
>     // checks and to avoid potential state change there.
>     XPCWrappedNative* wrapper =
>         XPCWrappedNative::GetWrappedNativeOfJSObject(nsnull, obj, nsnull, &obj2);
> 
>     if (wrapper) {
>         if (wrapper->IsValid())
>              TraceForValidWrapper(trc, wrapper);
>     } else if (obj2) {
>-        GetSlimWrapperProto(obj2)->TraceJS(trc);
>+        GetSlimWrapperProto(obj2)->TraceSelf(trc);
>     }
> }

So we've got a bit of a discrepancy here, where we (eventually) call TraceInside() for regular wrappers and TraceSelf() on the proto for slim wrappers. This makes sense (since there's nothing to trace on a slim wrapper except the proto), but wasn't immediately obvious to me when I read it. So it might be nice to have a comment saying something like "If we have a slim wrapper, there's nothing within the wrapper to trace. The only relevant part of XPCWrappedNative::TraceInside() is the call to TraceSelf() on the proto, which we do manually here".

> static void
> XPC_WN_Shared_Proto_Trace(JSTracer *trc, JSObject *obj)
> {
>     // This can be null if xpc shutdown has already happened
>     XPCWrappedNativeProto* p =
>         (XPCWrappedNativeProto*) xpc_GetJSPrivate(obj);
>     if (p)
>-        TraceScopeJSObjects(trc, p->GetScope());
>+        p->TraceInside(trc);
> }

If I understand correctly, shouldn't we be calling TraceSelf() here? I recognize that this isn't what the old code did, but that seems to be what the rest of this patch is moving to.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'll add the comments.

(In reply to Bobby Holley (:bholley) from comment #24)
> Comment on attachment 586311 [details] [diff] [review]
> trace scope from proto
> 
> So we've got a bit of a discrepancy here, where we (eventually) call
> TraceInside() for regular wrappers and TraceSelf() on the proto for slim
> wrappers. This makes sense (since there's nothing to trace on a slim wrapper
> except the proto), but wasn't immediately obvious to me when I read it. So
> it might be nice to have a comment saying something like "If we have a slim
> wrapper, there's nothing within the wrapper to trace. The only relevant part
> of XPCWrappedNative::TraceInside() is the call to TraceSelf() on the proto,
> which we do manually here".

It would be best if the slim wrapper case could call some sort of TraceInside method, and then that would call TraceSelf on the proto. That would make the slim wrapper and normal wrapper cases more similar. But since there isn't any kind of slim wrapper class to hang the TraceInside method off of, I did it this way.

Besides the comment, I could add some sort of static function with a name like TraceInsideSlimWrapper. It would just call TraceSelf on the proto. Do you think that would be clearer, or just pointless?

> > static void
> > XPC_WN_Shared_Proto_Trace(JSTracer *trc, JSObject *obj)
> > {
> >     // This can be null if xpc shutdown has already happened
> >     XPCWrappedNativeProto* p =
> >         (XPCWrappedNativeProto*) xpc_GetJSPrivate(obj);
> >     if (p)
> >-        TraceScopeJSObjects(trc, p->GetScope());
> >+        p->TraceInside(trc);
> > }
> 
> If I understand correctly, shouldn't we be calling TraceSelf() here? I
> recognize that this isn't what the old code did, but that seems to be what
> the rest of this patch is moving to.

Remember that the basic structure is for the trace hook to call TraceInside. This code is the trace hook for a WrappedNativeProto, so it should call proto->TraceInside (just as the trace hook for a wrapped native calls wrapper->TraceInside).

Comment 26

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #25)

> Besides the comment, I could add some sort of static function with a name
> like TraceInsideSlimWrapper. It would just call TraceSelf on the proto. Do
> you think that would be clearer, or just pointless?

Hm, good idea! I think that would clarify things a lot.

> 
> > > static void
> > > XPC_WN_Shared_Proto_Trace(JSTracer *trc, JSObject *obj)
> > > {
> > >     // This can be null if xpc shutdown has already happened
> > >     XPCWrappedNativeProto* p =
> > >         (XPCWrappedNativeProto*) xpc_GetJSPrivate(obj);
> > >     if (p)
> > >-        TraceScopeJSObjects(trc, p->GetScope());
> > >+        p->TraceInside(trc);
> > > }
> > 
> > If I understand correctly, shouldn't we be calling TraceSelf() here? I
> > recognize that this isn't what the old code did, but that seems to be what
> > the rest of this patch is moving to.
> 
> Remember that the basic structure is for the trace hook to call TraceInside.
> This code is the trace hook for a WrappedNativeProto, so it should call
> proto->TraceInside (just as the trace hook for a wrapped native calls
> wrapper->TraceInside).

Doh! My bad. I misread it as p->GetScope()->TraceInside(trc), since the old line operated on p->GetScope(). So yeah, the code is fine - ignore that comment. ;-)

Comment 27

[Bobby Holley (:bholley)]

Also, am I correct that scopes don't have a TraceInside() method because they don't have a trace callback on their JSObjects? I guess XPConnect doesn't necessarily control mGlobalJSObject and mPrototypeJSObject, so it can't rely on them having a trace hook.

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bobby Holley (:bholley) from comment #27)
> Also, am I correct that scopes don't have a TraceInside() method because
> they don't have a trace callback on their JSObjects? I guess XPConnect
> doesn't necessarily control mGlobalJSObject and mPrototypeJSObject, so it
> can't rely on them having a trace hook.

Yes, that's exactly correct. There's not much stuff that we trace for a scope, so it's not really a big deal.

Comment 29

[Bobby Holley (:bholley)]

Comment on attachment 586314 [details] [diff] [review]
eliminate special marking after GC marking

(In reply to Bill McCloskey (:billm) from comment #6)
> Created attachment 586314 [details] [diff] [review]
> eliminate special marking after GC marking
> 
> First, it passes around a flag to each GC
> callback saying whether it's a compartment GC.

This seems kind of lame. Wouldn't it be better to just expose this information on the runtime, either via jsapi or jsfriendapi? Ultimately your call, though.

> static JSDHashOperator
>+NativeInterfaceUnmarker(JSDHashTable *table, JSDHashEntryHdr *hdr,
>+                       uint32_t number, void *arg)
>+{
>+    XPCNativeInterface* iface = ((IID2NativeInterfaceMap::Entry*)hdr)->value;
>+    iface->Unmark();
>+    return JS_DHASH_NEXT;
>+}

These end up being a fair amount of duplicated code, and it wasn't clear to me initially that they were supposed to be "sweeping without destroying". What about passing a sentinel value as |arg| to Enumerate that would indicate that we should do this kind of behavior? Something like:

#define UNMARK_BUT_DONT_FINALIZE ((void*1)
(or UNMARK_BUT_DONT_DESTROY, or PRESERVE, or something of that sort).

>@@ -725,22 +742,16 @@ JSBool XPCJSRuntime::GCCallback(JSContex
>             printf("--------------------------------------------------------------\n");
>             int setsBefore = (int) self->mNativeSetMap->Count();
>             int ifacesBefore = (int) self->mIID2NativeInterfaceMap->Count();
> #endif
> 
>             // We use this occasion to mark and sweep NativeInterfaces,
>             // NativeSets, and the WrappedNativeJSClasses...
> 
>-            // Do the marking...
>-            XPCWrappedNativeScope::MarkAllWrappedNativesAndProtos();
>-
>-            self->mDetachedWrappedNativeProtoMap->
>-                Enumerate(DetachedWrappedNativeProtoMarker, nsnull);

As far as I can tell, this marking part was the only purpose of mDetachedWrappedNativeProtoMap. Can we eliminate it (in a followup patch)?


This patch is a pretty scary (but exciting!) change, because if there's ever a situation where a WN or proto lives in the map but isn't reachable from JS, we might destroy its set. So I think the final version of the patch should probably get sr from peterv.

Comment 30

[Bobby Holley (:bholley)]

Just getting back to this stuff now - sorry for the long wait. :-(

It's been long enough that I'd really like to go through the first 5 patches again before reviewing the last two. Can you update them to address the review comments so that I can have a better sense of the state of the world?

Comment 31

[Bobby Holley (:bholley)]

Comment on attachment 586315 [details] [diff] [review]
rename XPCWrappedNativeScope::TraceJS

cancelling review for now per comment 30.

Comment 32

[Bobby Holley (:bholley)]

Bill - I know you're busy, but can we at least get the r+ed stuff here landed? I need to add a new type of AutoMarkingPtr for bug 751454, but I don't want to bitrot your patches.

Comment 33

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, I landed the reviewed patches.

Comment:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ae4fa5a723

Mark bits:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d706cd6d6833

AutoMarkers:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72eb0eddb249

TraceJS changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14aa9de6287f

Comment 34

[Bobby Holley (:bholley)]

Awesome, thanks :-)

Comment 35

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/c9ae4fa5a723
http://hg.mozilla.org/mozilla-central/rev/d706cd6d6833
http://hg.mozilla.org/mozilla-central/rev/72eb0eddb249
http://hg.mozilla.org/mozilla-central/rev/14aa9de6287f

Comment 36

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I looked over the patches here, and I think it's time to make some progress. There are four patches not checked in here.

- "trace scope from proto" (attachment 586311 [details] [diff] [review]) - This one is important and I'd like to land it. I'll post a new version.

- "eliminate special marking after GC marking" (attachment 586314 [details] [diff] [review]) - An improved version of this patch landed in bug 758471, so this one can be obsoleted.

- "rename XPCWrappedNativeScope::TraceJS" (attachment 586315 [details] [diff] [review]) - This patch still applies. I'll reset the review flag.

- "patch to change AutoMarkingPtrs to use TraceSelf" (attachment 586316 [details] [diff] [review]) - This patch now looks kinda sketchy to me. I'll leave it here for future reference, but I think it may require significant rewriting to handle incremental GC correctly.

Comment 37

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: trace scope from proto, v2

I made the changes you asked for a while ago. It's not clear what the best place is for the comment about TraceSelf/TraceInside, so I just stuck it in XPCWrappedNativeJSOps.

Comment 38

[Bobby Holley (:bholley)]

Comment on attachment 586315 [details] [diff] [review]
rename XPCWrappedNativeScope::TraceJS

So, the fact that this is a static method that iterates over all scopes is a bit confusing, and I would prefer to reflect that somehow in the name.

TraceAllScopes? TraceWrappedNativesInAllScopes? TraceAllWrappedNatives? Something else? I'll let you pick.

Comment 39

[Bobby Holley (:bholley)]

Also, was the intention to replace all the TraceJS calls with TraceSelf (which guarantees that TraceInside will be called)?

Comment 40

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Note: These patches are landing in FF16, which is different from the target milestone!

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6ccaf417497
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2dfcf544b0

I took off the leave-open tag. Let's close this bug and deal with the final automarking patch in a separate bug, if we ever want to do that.

Regarding TraceJS elimination, that is a good goal. We'll need some more work on the automarkers to do that. I think that a scaled back version of the remaining patch here would do it.

Comment 41

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I think this is causing mozilla-inbound to burn:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ae2dfcf544b0

Snippet:

/usr/local/bin/ccache /usr/bin/g++-4.2 -arch i386 -o XPCJSWeakReference.o -c  -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DJSFILE -DJS_THREADSAFE -DEXPORT_XPC_API -DNO_NSPR_10_SUPPORT  -DMOZ_JSDEBUGGER -D_IMPL_NS_LAYOUT -I/builds/slave/m-in-osx-dbg/build/js/xpconnect/src/../wrappers -I/builds/slave/m-in-osx-dbg/build/js/xpconnect/src/../loader -I/builds/slave/m-in-osx-dbg/build/caps/include -I/builds/slave/m-in-osx-dbg/build/content/base/src -I/builds/slave/m-in-osx-dbg/build/content/events/src -I/builds/slave/m-in-osx-dbg/build/content/html/content/src -I/builds/slave/m-in-osx-dbg/build/content/html/document/src -I/builds/slave/m-in-osx-dbg/build/content/svg/content/src -I/builds/slave/m-in-osx-dbg/build/layout/style -I/builds/slave/m-in-osx-dbg/build/layout/base -I/builds/slave/m-in-osx-dbg/build/dom/base -I/builds/slave/m-in-osx-dbg/build/xpcom/ds  -I/builds/slave/m-in-osx-dbg/build/js/xpconnect/src -I. -I../../../dist/include  -I/builds/slave/m-in-osx-dbg/build/obj-firefox/dist/include/nspr -I/builds/slave/m-in-osx-dbg/build/obj-firefox/dist/include/nss      -fPIC  -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -isysroot /Developer/SDKs/MacOSX10.6.sdk -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fshort-wchar -pthread -DNO_X11 -pipe  -DDEBUG -D_DEBUG -DTRACING -g -O3 -fno-omit-frame-pointer    -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/XPCJSWeakReference.o.pp /builds/slave/m-in-osx-dbg/build/js/xpconnect/src/XPCJSWeakReference.cpp
../../../../js/xpconnect/src/XPCWrappedNativeJSOps.cpp: In function 'void MarkWrappedNative(JSTracer*, JSObject*)':
../../../../js/xpconnect/src/XPCWrappedNativeJSOps.cpp:659: error: cannot convert 'JSObject*' to 'JSTracer*' for argument '1' to 'void TraceInsideSlimWrapper(JSTracer*, JSObject*)'
make[6]: *** [XPCWrappedNativeJSOps.o] Error 1
make[6]: *** Waiting for unfinished jobs....
XPCWrapper.cpp
/usr/local/bin/ccache /usr/bin/g++-4.2 -arch i386 -o XPCWrapper.o -c  -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DJSFILE -DJS_THREADSAFE -DEXPORT_XPC_API -DNO_NSPR_10_SUPPORT  -DMOZ_JSDEBUGGER -D_IMPL_NS_LAYOUT -I/builds/slave/m-in-osx-dbg/build/js/xpconnect/src/../wrappers -I/builds/slave/m-in-osx-dbg/build/js/xpconnect/src/../loader -I/builds/slave/m-in-osx-dbg/build/caps/include -I/builds/slave/m-in-osx-dbg/build/content/base/src -I/builds/slave/m-in-osx-dbg/build/content/events/src -I/builds/slave/m-in-osx-dbg/build/content/html/content/src -I/builds/slave/m-in-osx-dbg/build/content/html/document/src -I/builds/slave/m-in-osx-dbg/build/content/svg/content/src -I/builds/slave/m-in-osx-dbg/build/layout/style -I/builds/slave/m-in-osx-dbg/build/layout/base -I/builds/slave/m-in-osx-dbg/build/dom/base -I/builds/slave/m-in-osx-dbg/build/xpcom/ds  -I/builds/slave/m-in-osx-dbg/build/js/xpconnect/src -I. -I../../../dist/include  -I/builds/slave/m-in-osx-dbg/build/obj-firefox/dist/include/nspr -I/builds/slave/m-in-osx-dbg/build/obj-firefox/dist/include/nss      -fPIC  -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -isysroot /Developer/SDKs/MacOSX10.6.sdk -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fshort-wchar -pthread -DNO_X11 -pipe  -DDEBUG -D_DEBUG -DTRACING -g -O3 -fno-omit-frame-pointer    -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/XPCWrapper.o.pp /builds/slave/m-in-osx-dbg/build/js/xpconnect/src/XPCWrapper.cpp
make[5]: *** [libs] Error 2
make[4]: *** [libs_tier_platform] Error 2
make[3]: *** [tier_platform] Error 2
make[2]: *** [default] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2
program finished with exit code 2
elapsedTime=472.968794
========= Finished compile failed (results: 2, elapsed: 7 mins, 57 secs) (at 2012-06-22 11:47:24.294891) =========

Comment 42

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47b99bc54fdf

I fixed the compile error in the try push, but not for inbound :-(. This fixes it.

Comment 43

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix for patch "trace scope from proto, v2"

I screwed up. When I rebased this patch from v1 to v2, I forgot to change one line.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6ccaf417497
https://hg.mozilla.org/mozilla-central/rev/ae2dfcf544b0
https://hg.mozilla.org/mozilla-central/rev/47b99bc54fdf

Comment 47

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e98b25f5fbc8

This is the last patch, so the bug can be closed when it merges.

Comment 48

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e98b25f5fbc8