482788 - (slimwrapper) Lightweight DOM wrappers

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Peter Van der Beken [:peterv]]

XPCWrappedNative wrappers are very general-purpose, and are able to deal with a wide variety of native objects. DOM objects have a number of restrictions that make XPCWrappedNative wrappers not a very good fit for them:
1) DOM objects are main-thread only (no need for locking)
2) DOM objects have only one XPCWrappedNative at a given moment and a number of DOM objects cache their wrapper in the object itself (no need for hashing)
3) DOM objects have quickstubs

We think it might give a performance benefit to have a new type of more lightweight wrappers for DOM objects, with only a JSObject, no "native" counterpart and not in any hashes. I've been working on this for the last couple of weeks, and it looks like the most promising approach is having wrappers that can morph on demand into a full XPCWrappedNative. We then make XPConnect code that needs a XPCWrappedNative morph a lightweight wrapper when it gets one, and we try to convert as much of that code as possible so it doesn't require a full XPCWrappedNative wrapper at all. Ideally the DOM quickstubs wouldn't need any XPCWrappedNative, I've also been able to make a number of the nsDOMClassInfo hooks not need one either (which helps a lot with avoiding morphing for .children[0], etc). I'm also looking at avoiding morphing when someone sets an expando, we currently need to morph because XPConnect needs to mark wrappers with expandos during GC and it needs to record an edge from the DOM object to its wrapper during CC, it does that by enumerating the wrappers in its hash.

Currently the DOMCI code uses PostCreate to set up the prototype chain of DOM classes. Because we don't call PostCreate for lightweight wrappers we need to set that up at a different time, turns out that the PostCreatePrototype hook that Jason added is perfect for that. Because that is a fairly self-contained change I'll file a seperate bug for that.

Comment 1

[Boris Zbarsky [:bzbarsky]]

As far as expandos go, could we record CC edges via nsINode instead?  We have a pointer to the wrapper there, right?  Not sure about marking wrappers with expandos for GC, since we're not walking the DOM for that...

Comment 2

[Peter Van der Beken [:peterv]]

The problem is mostly making sure that all the DOM wrapper JS objects with expandos either get marked during GC and get an edge recorded to them from the DOM object (nsINode or other) during CC. So that probably means storing DOM objects with wrappers with expandos in a hash somewhere.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: WIP

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1 (diff -w)

Let's start the reviews. Mochitests pass, but I'm still making sure it doesn't leak. There's some things that I don't like, but we'll do them as follow-up bugs.

With this patch we create slim wrappers for objects that have a cached wrapper and a scriptable helper that explicitly allows slim wrappers from its PreCreate hook. Currently we allow it for nodelists and nodes, except for documents, elements that have an XBL binding and plugin elements. We could allow it for all elements too at some point, but I'd rather do that in another patch. If we get to a point where we need a normal XPCWrappedNative, we morph. That currently still happens if someone calls a non-quickstubbed method or gets a non-quickstubbed property. We could fix that, but again I'd rather do that in another patch. We don't morph very many wrappers with this patch anyway (in general <10%). If a scriptable helper allows slim wrappers it's supposed to be able to deal with a null wrapper argument to many of its hooks (need to add some comments documenting all this). I've changed the DOMCI hooks that needed to be changed.

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 7

[Boris Zbarsky [:bzbarsky]]

On the test in bug 500489, the non-innerHTML parts, this gives us a 25% speedup.  It significantly speeds up the xpc_qsXPCOMObjectToJsval part...

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 385448 [details] [diff] [review]
v1 (diff -w)

- In nsScriptSecurityManager::doGetObjectPrincipal():

         // NOTE: These class and getObjectOps hook checks better match
         // what IS_WRAPPER_CLASS() does in xpconnect!
-        if (jsClass == sXPCWrappedNativeJSClass ||
-            jsClass->getObjectOps == sXPCWrappedNativeGetObjOps1 ||
-            jsClass->getObjectOps == sXPCWrappedNativeGetObjOps2) {
-            nsIXPConnectWrappedNative *xpcWrapper =
-                (nsIXPConnectWrappedNative *)caps_GetJSPrivate(aObj);
 
-            if (xpcWrapper) {
+        JSEqualityOp op =
+            (jsClass->flags & JSCLASS_IS_EXTENDED) ?
+            reinterpret_cast<const JSExtendedClass*>(jsClass)->equality :
+            nsnull;
+        if (op == sXPCWrappedNativeEqualityOps ||
+            op == sXPCSlimWrapperEqualityOps) {
+            result = sXPConnect->GetPrincipal(cx, aObj,

Update the comment above this change.

- In BaseStubConstructor():

   nsCOMPtr<nsIXPConnectJSObjectHolder> holder;
-  rv = nsDOMGenericSH::WrapNative(cx, obj, native, rval,
-                                  getter_AddRefs(holder));
+  rv = nsDOMGenericSH::WrapNative(cx, obj, native, rval);
 
   return rv;

Looks like holder can be removed here.

- In nsDOMClassInfo.h:

+  static inline nsISupports* GetNative(nsIXPConnectWrappedNative *wrapper,
+                                       JSContext *cx, JSObject *obj)
+  {
+    return wrapper ? wrapper->Native() :
+                     static_cast<nsISupports*>(JS_GetPrivate(cx, obj));
+  }

If I do foo = {}; foo.__proto__ = document.body(); foo.someprop; we'll likely end up in classinfo hooks with obj being foo, and being passed directly into here. Seems like this method needs to at least check that obj's class flags include JSCLASS_PRIVATE_IS_NSISUPPORTS. If that's not enough here, this could walk the proto chain until it finds a known class, or something.

- In nsXPConnect.cpp:

+/* nsIXPConnectWrappedNative getWrappedNativeOfJSObject (in JSContextPtr aJSContext, in JSObjectPtr aJSObj); */
+NS_IMETHODIMP
+nsXPConnect::GetJSObjectOfWrapper(JSContext * aJSContext,
+                                  JSObject * aJSObj,
+                                  JSObject **_retval)

Update the comment above this method, or remove it if you want to.

That's all for now, still need to read through from xpcquickstubs.cpp to the end...

Comment 9

[Johnny Stenback (:jst)]

(In reply to comment #8)
> If I do foo = {}; foo.__proto__ = document.body(); foo.someprop;

I of course meant foo.__proto__ = document.body; there.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 385448 [details] [diff] [review]
v1 (diff -w)

- In GetMemberInfo():

     NS_ASSERTION(IS_WRAPPER_CLASS(STOBJ_GET_CLASS(obj)) ||
-                 STOBJ_GET_CLASS(obj) == &XPC_WN_Tearoff_JSClass,
-                 "obj must be an XPCWrappedNative");
+                 STOBJ_GET_CLASS(obj) == &XPC_WN_Tearoff_JSClass ||
+                 IS_SLIM_WRAPPER_CLASS(STOBJ_GET_CLASS(obj)),
+                 "obj must be an wrapper");

Typo, must be "a" wrapper.

- In xpcwrappednative.cpp:

+#ifdef DEBUG
+static PRUint32 morphedSlimWrappers;

sMorphedSlimWrappers?

- In ConstructSlimWrapper():

+    if(parent != plannedParent)
+    {
+        XPCWrappedNativeScope *newXpcScope =
+            XPCWrappedNativeScope::FindInJSObjectScope(ccx, parent);
+        JSBool sameOrigin;
+        if(!xpc_SameScope(newXpcScope, xpcScope, &sameOrigin))

Do you really care whether we're crossing origins here? If not you could simply make this check xpcScope != newXpcScope.

- In xpcwrappednativejsops.cpp:

+#define WRAPPER_SLOTS (JSCLASS_HAS_PRIVATE | 

You told me on irc that the extra slots is needed for slim wrappers to have fast access to their XPCWrappedNativeProto in GetSlimWrapperProto(), and that this slot here is only needed for the classes to match between wrapped natives and slim wrappers.

I wonder if it wouldn't be as fast to get the proto in GetSlimWrapperProto() by first getting the slim wrappers proto (STOBJ_GET_PROTO()) followed by an STOBJ_GET_PRIVATE() on the proto toget the XPCWrappedNativeProto? For this to be safe we'd need to be able to guarantee that a slim wrapper's proto is never changed. Food for thought, and doing this in a followup bug would be fine with me, assuming that it's a reasonable thing to do.

More later, need sleep.

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to comment #8)
> +  static inline nsISupports* GetNative(nsIXPConnectWrappedNative *wrapper,
> +                                       JSContext *cx, JSObject *obj)
> +  {
> +    return wrapper ? wrapper->Native() :
> +                     static_cast<nsISupports*>(JS_GetPrivate(cx, obj));
> +  }
> 
> If I do foo = {}; foo.__proto__ = document.body(); foo.someprop; we'll likely
> end up in classinfo hooks with obj being foo, and being passed directly into
> here. Seems like this method needs to at least check that obj's class flags
> include JSCLASS_PRIVATE_IS_NSISUPPORTS. If that's not enough here, this could
> walk the proto chain until it finds a known class, or something.

As discussed on IRC, I think we're protected by PRE_HELPER_STUB which will make sure the SH hooks either get a XPCNativeWrapper or a slim wrapper. It would throw if we get a slim wrapper on the proto chain, maybe we should morph. I'd like to file a follow-up bug to sort that out.

(In reply to comment #10)
> - In ConstructSlimWrapper():
> 
> +    if(parent != plannedParent)
> +    {
> +        XPCWrappedNativeScope *newXpcScope =
> +            XPCWrappedNativeScope::FindInJSObjectScope(ccx, parent);
> +        JSBool sameOrigin;
> +        if(!xpc_SameScope(newXpcScope, xpcScope, &sameOrigin))
> 
> Do you really care whether we're crossing origins here? If not you could simply
> make this check xpcScope != newXpcScope.

I originaly did that, but then decided to go with what NativeInterface2JSObject did. I don't feel strongly about this. We would potentially get less slim wrappers, right?

> I wonder if it wouldn't be as fast to get the proto in GetSlimWrapperProto() by
> first getting the slim wrappers proto (STOBJ_GET_PROTO()) followed by an
> STOBJ_GET_PRIVATE() on the proto toget the XPCWrappedNativeProto? For this to
> be safe we'd need to be able to guarantee that a slim wrapper's proto is never
> changed. Food for thought, and doing this in a followup bug would be fine with
> me, assuming that it's a reasonable thing to do.

I originally did it that way, but went with the slot to not have to worry about the proto changing. If we could easily detect the proto changing and morph at that point I'd be in favor of that.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 385448 [details] [diff] [review]
v1 (diff -w)

- In XPC_WN_Shared_ToString():

+    if(IS_SLIM_WRAPPER_CLASS(JS_GET_CLASS(cx, obj)))
+    {
+        XPCWrappedNativeProto *p = GetSlimWrapperProto(cx, obj);
+        XPCNativeScriptableInfo *si = p ? p->GetScriptableInfo() : nsnull;
+        if(si)

Can we ever have a slim wrapper and not have scriptable info?

+#ifdef DEBUG_slimwrappers
+static PRUint32 finalizedSlimWrappers;

gFinalizedSlimWrappers?

- In XPC_SWN_ClearWrapper():

Shouldn't this be named XPC_SWN_Finalize?

-GetScopeOfObject(JSObject* obj)
+GetScopeOfObject(JSContext* cx, JSObject* obj)
 {
     nsISupports* supports;
     JSClass* clazz = STOBJ_GET_CLASS(obj);
 
+    if(IS_SLIM_WRAPPER_CLASS(clazz))
+        return GetSlimWrapperProto(cx, obj)->GetScope();

The only reason you need the context argument here is that you need ot pass it to GetSlimWrapperProto(), but GetSlimWrapperProto() doesn't actually need a context argument. It only passes it to JS_GET_CLASS, but you could, and should, use STOBJ_GET_CLASS instead, which doesn't need a context.

Changing this would let you undo a bunch of the method signature changes it seems, at least GetMemberInfo(), doGetObjectPrincipal(), nsXPConnect::GetPrincipal() wouldn't need a cx arg, CheckXPCPermissions(), and probably several more.

Also, as I was looking into this, I realized that GetNative() in nsDOMClassInfo.h doesn't need a cx arg either, you could copy xpc_GetJSPrivate() into nsDOMClassInfo.h (and rename it) and use it, which doesn't need a context either. Which means that do_QueryWrappedNative() doesn't need a cx either.

In fact, you should do s/JS_GetPrivate/xpc_GetJSPrivate/ on the xpconnect part of the patch, and s/JS_GetParent/STOBJ_GET_PARENT/ on the whole patch.

r=jst with my comments looked into.

Comment 13

[Johnny Stenback (:jst)]

(In reply to comment #10)
> +#ifdef DEBUG
> +static PRUint32 morphedSlimWrappers;
> 
> sMorphedSlimWrappers?

Actually, that suggestion should've been gMorphedSlimWrappers.

Comment 14

[Johnny Stenback (:jst)]

(In reply to comment #11)
> (In reply to comment #8)
> > +  static inline nsISupports* GetNative(nsIXPConnectWrappedNative *wrapper,
> > +                                       JSContext *cx, JSObject *obj)
> > +  {
> > +    return wrapper ? wrapper->Native() :
> > +                     static_cast<nsISupports*>(JS_GetPrivate(cx, obj));
> > +  }
> > 
> > If I do foo = {}; foo.__proto__ = document.body(); foo.someprop; we'll likely
> > end up in classinfo hooks with obj being foo, and being passed directly into
> > here. Seems like this method needs to at least check that obj's class flags
> > include JSCLASS_PRIVATE_IS_NSISUPPORTS. If that's not enough here, this could
> > walk the proto chain until it finds a known class, or something.
> 
> As discussed on IRC, I think we're protected by PRE_HELPER_STUB which will make
> sure the SH hooks either get a XPCNativeWrapper or a slim wrapper. It would
> throw if we get a slim wrapper on the proto chain, maybe we should morph. I'd
> like to file a follow-up bug to sort that out.

Yeah, followup bug is fine. But please do add a comment in GetNative() as to why this is safe.

> 
> (In reply to comment #10)
> > - In ConstructSlimWrapper():
> > 
> > +    if(parent != plannedParent)
> > +    {
> > +        XPCWrappedNativeScope *newXpcScope =
> > +            XPCWrappedNativeScope::FindInJSObjectScope(ccx, parent);
> > +        JSBool sameOrigin;
> > +        if(!xpc_SameScope(newXpcScope, xpcScope, &sameOrigin))
> > 
> > Do you really care whether we're crossing origins here? If not you could simply
> > make this check xpcScope != newXpcScope.
> 
> I originaly did that, but then decided to go with what NativeInterface2JSObject
> did. I don't feel strongly about this. We would potentially get less slim
> wrappers, right?

Looking at xpc_SameScope() I don't see how xpcScope != newXpcScope vs !xpc_SameScope() would change the number of wrappers we create. I can't say I feel strongly here either, but it'd be less code and we'd never waste time getting and comparing principals if we just compare the scopes directly.

> > I wonder if it wouldn't be as fast to get the proto in GetSlimWrapperProto() by
> > first getting the slim wrappers proto (STOBJ_GET_PROTO()) followed by an
> > STOBJ_GET_PRIVATE() on the proto toget the XPCWrappedNativeProto? For this to
> > be safe we'd need to be able to guarantee that a slim wrapper's proto is never
> > changed. Food for thought, and doing this in a followup bug would be fine with
> > me, assuming that it's a reasonable thing to do.
> 
> I originally did it that way, but went with the slot to not have to worry about
> the proto changing. If we could easily detect the proto changing and morph at
> that point I'd be in favor of that.

We can easily detect if JS does obj.__proto__ = foo, but we can't easily detect if someone uses the JS API to change the proto. The former is critical, of course, but the latter I don't know if we need to worry about to be honest. But no need to stress about this for initial landing. Maybe file a followup bug to think this through in?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 385448 [details] [diff] [review]
v1 (diff -w)

I noticed one case of |else {| in xpconnect that you should probably fix. Other than that, this looks good.

Comment 16

[Peter Van der Beken [:peterv]]

I'll attach a patch that takes care of the comments, but I'm tracking down a leak during mochitests. It's an intermittent leak, seems to be related to XBL, but I'm having a hard time figuring out what's going wrong.

Comment 17

[Peter Van der Beken [:peterv]]

I think I have a patch that fixes the leak. Will file a new bug for that tomorrow.

Comment 18

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/b43290ec9366
http://hg.mozilla.org/mozilla-central/rev/cd23e1bd82c4

Comment 19

[Daniel Holbert [:dholbert]]

FWIW, changeset b43290ec9366 seems to have gotten us 0.63% and 0.59% improvements on DHTML in XP and Vista, respectively:
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/fd4562571938c475
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/214bbb0197de60b4

...and a 0.68% win on tp4 on OS X.
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/479b72f30b7395e6