Closed Bug 590612 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: XPConnect, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: bzbarsky, Assigned: peterv)

References

Details

Attachments

(1 file, 4 obsolete files)

See bug 585783; want something similar here.
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.
Peter, want to just take this bug, then?
Attached patch v1 (obsolete) — Splinter Review
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?
Assignee: bzbarsky → peterv
Status: NEW → ASSIGNED
Worth double-checking how this affects the quickstub microbenchmark numbers...

xpconnect_utils.h?  Or XPConnectUtils.h?
This speeds up dom-query by about 10%.
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.
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....
Checking something now to test this last theory.
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.
(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?
Yeah, if we have to.  I probably wouldn't worry about it for now.
Attached patch v1 (obsolete) — Splinter Review
Attachment #469164 - Attachment is obsolete: true
Attachment #469615 - Attachment is obsolete: true
Attachment #475224 - Flags: review?(bzbarsky)
Attached patch v1 (obsolete) — Splinter Review
Forgot to qref.
Attachment #475224 - Attachment is obsolete: true
Attachment #475238 - Flags: review?(bzbarsky)
Attachment #475224 - Flags: review?(bzbarsky)
Comment on attachment 475238 [details] [diff] [review]
v1

r=me
Attachment #475238 - Flags: review?(bzbarsky) → review+
Attachment #475238 - Flags: approval2.0?
Comment on attachment 475238 [details] [diff] [review]
v1

Yes, a=jst!
Attachment #475238 - Flags: approval2.0? → approval2.0+
blocking2.0: --- → ?
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.
blocking2.0: ? → beta8+
blocking2.0: beta8+ → betaN+
Attached patch v1.1Splinter Review
Attachment #475238 - Attachment is obsolete: true
Attachment #490884 - Flags: review?(bzbarsky)
I'll file a bug on removing GetXPCWrappedNativeJSClassInfo, I removed the only caller but I guess we're in API freeze.
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.
Attachment #490884 - Flags: review?(bzbarsky) → review+
Blocks: 533634
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.
Attachment #490884 - Flags: approval2.0?
Attachment #490884 - Flags: approval2.0? → approval2.0+
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
this patch landed:

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

peter, marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 533634
You need to log in before you can comment on or make changes to this bug.