590612 - Speed up js-wrapping in classinfo when we already have a wrapper

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 585783; want something similar here.

Comment 1

[Peter Van der Beken [:peterv]]

I have a patch that does this, but need to clean it up. Probably need to add a header that we can include from both XPConnect and DOM so we can but shared code in there.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Peter, want to just take this bug, then?

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1

Something like this (haven't tested yet, but it compiles). This introduces a new exported xpconnect.h header. I don't like that it's exported but given that nsContentUtils.h is included all over the place, if we do want to add an inline function to it this seems like the only option. I also don't like the name, but don't have any better ideas. Suggestions?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Worth double-checking how this affects the quickstub microbenchmark numbers...

xpconnect_utils.h?  Or XPConnectUtils.h?

Comment 5

[Peter Van der Beken [:peterv]]

This speeds up dom-query by about 10%.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Right; dom-query is hitting this codepath but not the quickstubbed one much.  If you change the firstChild test in dromaeo to use nextSibling to iterate, not childNodes[n], then you'd only hit the old codepath...

I'll try running this patch on that locally to see what the numbers are like.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch I needed to make this compile

With this patch, my score on Dromaeo's firstChild test went from 95 to 133.

However if I modify the firstChild test to avoid childNodes and stick to firstChild/nextSibling access, then my score without this patch is 211, whereas with this patch it's 200 (both +-2 or so, and repeatably over several tests).

Presumably the extra null-check isn't being optimized away when we inline or something....

Comment 8

[Boris Zbarsky [:bzbarsky]]

Checking something now to test this last theory.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Looks like compiling with -fomit-frame-pointer pretty much gets the performance back on Mac, and since we plan to do that anyway, I'm tempted to let this be.

Comment 10

[Peter Van der Beken [:peterv]]

(In reply to comment #7)
> Presumably the extra null-check isn't being optimized away when we inline or
> something....

Bah. Worst case we make it a macro?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, if we have to.  I probably wouldn't worry about it for now.

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v1

Forgot to qref.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 475238 [details] [diff] [review]
v1

r=me

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 475238 [details] [diff] [review]
v1

Yes, a=jst!

Comment 16

[Peter Van der Beken [:peterv]]

BTW, I tried landing this but got type conflicts on Windows (probably because nsContentUtils.h is included all over the place). The plan is to have a DOMCI-specific WrapNative method that does the fast path, and leave the nsContentUtils one alone.

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 18

[Peter Van der Beken [:peterv]]

I'll file a bug on removing GetXPCWrappedNativeJSClassInfo, I removed the only caller but I guess we're in API freeze.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 490884 [details] [diff] [review]
v1.1

>+++ b/caps/src/nsScriptSecurityManager.cpp
>@@ -2413,17 +2407,17 @@ nsScriptSecurityManager::doGetObjectPrin
>         // NOTE: These class and equality hook checks better match
>         // what IS_WRAPPER_CLASS() does in xpconnect!

This comment needs to go away.

r=me with that.

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 490884 [details] [diff] [review]
v1.1

Performance improvement. Previous patch had approval, but needed to be backed out because it broke Windows builds. This one passes on try.

Comment 21

[Doug Turner (:dougt)]

this broke Qt builds.  slots is already defined on Qt (they have this idea of slots and signals).  I think we need to undefine slots in jsobj.h.  F.e.:

#ifdef MOZ_ENABLE_QT
 #undef slots
#endif

Comment 22

[Doug Turner (:dougt)]

this patch landed:

http://hg.mozilla.org/mozilla-central/rev/7e42ccaa7269

peter, marking fixed.