Closed Bug 457897 Opened 16 years ago Closed 16 years ago

Remove QI on 'this' object when calling from JS to C++

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: jorendorff, Assigned: peterv)

References

Details

Attachments

(1 file, 7 obsolete files)

Forked off from bug 453738 comment 49 by jst.

Right now we QueryInterface to the desired interface, then later Release.  This is slow (see bug 453738 comment 47 for an extreme case--64% of execution time).

Peterv and I have patches for this, forthcoming...
Attached patch Add offsets to nodes (obsolete) — Splinter Review
Attached patch Add offsets to nodes (obsolete) — Splinter Review
Attachment #341164 - Attachment is obsolete: true
This patch isn't right.  Posting so peterv can grab it and debug.

In a debug browser build, Mochitests hit an assertion pretty quickly:

#6  0x00467a81 in NS_DebugBreak_P (aSeverity=3, aStr=0xa2bf61f "we rather crash than hang", aExpr=0xa2cb3fc "!(mState & XML_HTTP_REQUEST_SYNCLOOPING)", aFile=0xa2ca874 "content/base/src/nsXMLHttpRequest.cpp", aLine=1055) at xpcom/base/nsDebugImpl.cpp:327
#7  0x09d2c666 in nsXMLHttpRequest::~nsXMLHttpRequest (this=0xf899e30) at content/base/src/nsXMLHttpRequest.cpp:1055
#8  0x09d1fd74 in nsXHREventTarget::Release (this=0xf899e30) at content/base/src/nsXMLHttpRequest.cpp:576
#9  0x09d20600 in nsXMLHttpRequest::Release (this=0xf899e30) at content/base/src/nsXMLHttpRequest.cpp:1215
#10 0x0c50ba88 in XPCJSRuntime::GCCallback (cx=0x932200, status=JSGC_END) at js/src/xpconnect/src/xpcjsruntime.cpp:818
#11 0x09f26890 in DOMGCCallback (cx=0x932200, status=JSGC_END) at dom/src/base/nsJSEnvironment.cpp:3582
#12 0x0c4db73f in XPCCycleCollectGCCallback (cx=0x932200, status=JSGC_END) at js/src/xpconnect/src/nsXPConnect.cpp:459
#13 0x0020c60b in js_GC (cx=0x932200, gckind=GC_NORMAL) at js/src/jsgc.cpp:3734
#14 0x001bf8e7 in JS_GC (cx=0x932200) at js/src/jsapi.cpp:2474
#15 0x0c4d7cb3 in nsXPConnect::Collect (this=0x743e40) at js/src/xpconnect/src/nsXPConnect.cpp:530
#16 0x0047002d in nsCycleCollector::Collect (this=0x670000, aTryCollections=1) at /Users/jason/dev/moz/mozilla-central-dev/xpcom/base/nsCycleCollector.cpp:2256

The memory management in this patch is pretty goofy, because we have completely different ways of making sure the object stays alive for the duration of the call, depending on where we find the pointer.  There might be a bug, or it may trigger a bug in cycle collector.
I wonder if that assertion is bug 458206.
Attached patch v1 (obsolete) — Splinter Review
Jason, I tried hooking up xpc_qsUnwrapThisFromCcx too, and it seems to work. Could you look at those changes and let me know if they are ok (especially the use of &vp[1]).
Assignee: nobody → peterv
Attachment #341343 - Attachment is obsolete: true
Attachment #341537 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Everything in xpcquickstubs and qsgen.py looks fine, with a style nit or two:

in xpcquickstubs.cpp:
> +nsresult
> +xpc_getNativeFromWrapper(XPCWrappedNative *wrapper,

It looks like that should be |static| and shouldn't have the xpc_ prefix.

in qsgen.py:
> +        # Undocumented, but the interpreter puts 'this' at argv[-1],
> +        # which is vp[1]; and it's ok to overwrite it.

This appears twice; could you change the second one to just
   # as above, ok to overwrite vp[1]
and stick it at the end of the line it's commenting?

More significantly, I wonder whether attaching ThisPtrOffsets to the WNProto is correct.  Within a page, does each C++ class get its own WNProto?  If so, the patch is fine.  If there one WNProto per ClassInfo, and sometimes multiple classes share an nsDOMClassInfo instance, then this could get us into trouble: we could apply one class's ThisPtrOffsets to an object of a different class.
Multiple classes can certainly share one classinfo at the moment.  For example, content lists of various sorts have different C++ impls but a lot of them use the HTMLCollection classinfo.
Then it certainly seems like this could crash if you try to do something with objects of both classes.  Does it?

The obvious workaround would be to land something that QIs to ThisPtrOffsets in UnwrapThis.  Another way would be to cache the ThisPtrOffsets in the XPCWN rather than the WNProto.

Another way would be to share WNProtos based on a common (ClassInfo, ThisPtrOffsets) pair, rather than just ClassInfo.  I'm not sure I like that, though.  Apart from being a lot of work, the change would be visible to scripts that touch prototypes.
Yes, I forgot to do something about that. Originally we only added the offsets to nodes, which AFAICT all have their own specific classinfo.
I tried removing the caching and it is a little bit faster with caching, we could just remove the caching if we don't care. Or we could rely on the fact that nodes have a unique WPNProto per class and check the flags for CONTENT_OBJECT, and fallback to always QIing for DOM_OBJECT.
I have patches for both approaches, will attach.

BTW, this patch on its own doesn't give a huge speedup, the numbers aren't super stable but I get about 4.5% on Dromaeo (and about 2.5% without the caching). Combined with the cached wrappers patch we get near 20% though.
And no, I haven't gotten it to crash yet, maybe we got lucky and the offsets are identical in the cases that we hit?
For debug builds we can add an NS_ASSERTION that makes sure the cached table is always the same as the one gotten from QIing the native when we create a new wrapper.

If we want to enforce this at runtime we could always QI when creating a wrapper and clear the cached table if it's a different one. We'd then fall back to always QIing for the wrappers sharing that proto.

Still thinking if there's a way to enforce this at compile time.
Attached patch v1.1 (obsolete) — Splinter Review
I think I want to do this for now, cache the offset table in the WNP for nsIContent-derived nodes. This leaves out attribute and document nodes and nodelists. We'll still use offsets for those, but we'll QI every time we need the table. I've also added an assertion to make sure the table is always the same as any cached table in the WNP.
Attachment #342489 - Attachment is obsolete: true
Attached patch v1.1 (alternate) (obsolete) — Splinter Review
This one never caches the offset table.
Note that these patches contain a small fix to add the nsIDOMSVGElement interface to nsSVGDefsElement's interface list. nsIDOMSVGDefsElement inherits from nsIDOMSVGElement, so it was definitely an oversight.

I would file a followup bug to figure out how to start using the cache for attributes, documents and nodelists without running into the issue mentioned in comment 7 (preferably with compile-time checks).
Comment on attachment 342632 [details] [diff] [review]
v1.1 (alternate)

- In GetMethodInfo(), in xpcquickstubs.cpp:

+    GetMemberInfo(JSVAL_TO_OBJECT(vp[-1]), methodId, ifaceName, memberName);

This is called by fast native methods AFAICT, vp points to the beginning of the jsval array in that case (as opposed to in the slow native method case), so what's this actually doing then? Don't you want vp[1], which you fill in yourself before you get here? Or if you're not guaranteed to get here through the code that does that, you'll need to use JS_ComputeThis() here.

- In getNativeFromWrapper():

+    // Try using the QITableEntry to avoid the extra AddRef and Release.
+    QITableEntry* entries = wrapper->GetOffsets();
+    if(entries)
+    {
+        for(QITableEntry* e = entries; e->iid; e++)
+        {
+            if(e->iid->Equals(iid))

I'd love to see us somehow hardcode the offset into the QI table entry in the quickstubs code somehow so we wouldn't need this loop, but I don't really know how to do it w/o actually having to hand code that and somehow sanity check that at build time or something.

I didn't have time to go through all of this (especially not all the QI implementation changes), but what I did look through looked good!
(In reply to comment #15)
> (From update of attachment 342632 [details] [diff] [review])
> - In GetMethodInfo(), in xpcquickstubs.cpp:
> 
> +    GetMemberInfo(JSVAL_TO_OBJECT(vp[-1]), methodId, ifaceName, memberName);
> 
> This is called by fast native methods AFAICT, vp points to the beginning of the
> jsval array in that case (as opposed to in the slow native method case), so
> what's this actually doing then? Don't you want vp[1], which you fill in
> yourself before you get here? Or if you're not guaranteed to get here through
> the code that does that, you'll need to use JS_ComputeThis() here.

Yep, my mistake.  This should be vp[1].  Similarly in the comment in xpcquickstubs.h.

There should be a comment on xpc_qsThrowMethodFailed and xpc_qsThrowBadArg requiring that vp[1] already be set by a previous call to xpc_qsUnwrapThis{,FromCcx}.

> +    // Try using the QITableEntry to avoid the extra AddRef and Release.
> +    QITableEntry* entries = wrapper->GetOffsets();
> +    if(entries)
> +    {
> +        for(QITableEntry* e = entries; e->iid; e++)
> +        {
> +            if(e->iid->Equals(iid))
> 
> I'd love to see us somehow hardcode the offset into the QI table entry in the
> quickstubs code somehow so we wouldn't need this loop, but I don't really know
> how to do it w/o actually having to hand code that and somehow sanity check
> that at build time or something.

I know two techniques that don't involve hand coding the tables.  The first one comes from an IBM "Jalapeño" paper; the second one Andreas told me about last week.  :)

- If you can arrange to only care about C++ classes that form a single-inheritance sub-tree of the class hierarchy, then the table can just be a display of class id-codes, and the offset is always zero.  (You can *almost* do this with the DOM, but not quite.  I tried.)

- To support multiple inheritance, each QI table could be a perfect hash table. (Each one would have its own hashing function parameters, calculated at build time, by a script, to avoid collisions.  Each table would be small, so we wouldn't need to waste a ton of space or build-time CPU to do it.)

Either way you can probably substitute 32-bit identifiers for the IIDs for another little win.  I'd like to see us do this stuff for all scriptable objects in Mozilla 2.
Attached patch v1.2 (obsolete) — Splinter Review
Small correction to nsXULElement QI and the vp[1] correction from comment 15.
Blake, could you look over the JS bits? Jonas, could you review the QI changes?
Attachment #342631 - Attachment is obsolete: true
Attachment #342632 - Attachment is obsolete: true
Attachment #343243 - Flags: superreview?(jonas)
Attachment #343243 - Flags: review?(mrbkap)
Comment on attachment 343243 [details] [diff] [review]
v1.2

r+sr=jst!
Attachment #343243 - Flags: superreview?(jonas)
Attachment #343243 - Flags: superreview+
Attachment #343243 - Flags: review?(mrbkap)
Attachment #343243 - Flags: review+
Attached patch v1.2Splinter Review
Patch that was landed.
Attachment #343243 - Attachment is obsolete: true
Attachment #346036 - Flags: superreview+
Attachment #346036 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Depends on: 462926
Depends on: 462929
You need to log in before you can comment on or make changes to this bug.