Closed
Bug 590612
Opened 14 years ago
Closed 14 years ago
Speed up js-wrapping in classinfo when we already have a wrapper
Categories
(Core :: XPConnect, defect, P1)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: bzbarsky, Assigned: peterv)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
jst
:
approval2.0+
|
Details | Diff | Splinter Review |
See bug 585783; want something similar here.
Assignee | ||
Comment 1•14 years ago
|
||
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.
Reporter | ||
Comment 2•14 years ago
|
||
Peter, want to just take this bug, then?
Assignee | ||
Comment 3•14 years ago
|
||
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
Reporter | ||
Comment 4•14 years ago
|
||
Worth double-checking how this affects the quickstub microbenchmark numbers...
xpconnect_utils.h? Or XPConnectUtils.h?
Assignee | ||
Comment 5•14 years ago
|
||
This speeds up dom-query by about 10%.
Reporter | ||
Comment 6•14 years ago
|
||
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.
Reporter | ||
Comment 7•14 years ago
|
||
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....
Reporter | ||
Comment 8•14 years ago
|
||
Checking something now to test this last theory.
Reporter | ||
Comment 9•14 years ago
|
||
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.
Assignee | ||
Comment 10•14 years ago
|
||
(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?
Reporter | ||
Comment 11•14 years ago
|
||
Yeah, if we have to. I probably wouldn't worry about it for now.
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #469164 -
Attachment is obsolete: true
Attachment #469615 -
Attachment is obsolete: true
Attachment #475224 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•14 years ago
|
||
Forgot to qref.
Attachment #475224 -
Attachment is obsolete: true
Attachment #475238 -
Flags: review?(bzbarsky)
Attachment #475224 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 14•14 years ago
|
||
Comment on attachment 475238 [details] [diff] [review]
v1
r=me
Attachment #475238 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #475238 -
Flags: approval2.0?
Comment 15•14 years ago
|
||
Comment on attachment 475238 [details] [diff] [review]
v1
Yes, a=jst!
Attachment #475238 -
Flags: approval2.0? → approval2.0+
Reporter | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Assignee | ||
Comment 16•14 years ago
|
||
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.
Updated•14 years ago
|
blocking2.0: ? → beta8+
Updated•14 years ago
|
blocking2.0: beta8+ → betaN+
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #475238 -
Attachment is obsolete: true
Attachment #490884 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 18•14 years ago
|
||
I'll file a bug on removing GetXPCWrappedNativeJSClassInfo, I removed the only caller but I guess we're in API freeze.
Reporter | ||
Comment 19•14 years ago
|
||
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+
Assignee | ||
Comment 20•14 years ago
|
||
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?
Updated•14 years ago
|
Attachment #490884 -
Flags: approval2.0? → approval2.0+
Comment 21•14 years ago
|
||
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•14 years ago
|
||
this patch landed:
http://hg.mozilla.org/mozilla-central/rev/7e42ccaa7269
peter, marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•