Closed
Bug 684435
Opened 13 years ago
Closed 13 years ago
Nix class equality hooks on DOM objects where possible
Categories
(Core :: XPConnect, defect, P1)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: perf)
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
Everything that has a unique XPCWN doesn't need one.
Assignee | ||
Comment 1•13 years ago
|
||
81 #define IS_WRAPPER_CLASS(clazz) \
82 (clazz->ext.equality == js::Valueify(XPC_WN_Equality))
This will need to change.... :(
Assignee | ||
Comment 2•13 years ago
|
||
Luckily, we seem to have a clazz->ext.isWrappedNative now!
Assignee | ||
Comment 3•13 years ago
|
||
Assignee | ||
Comment 4•13 years ago
|
||
So I hacked off the equality hook for HTMLHtmlElement and HTMLDocument. Then I get the following times on the attached testcase:
Hook, TI+JM: 305
Hook, JM+TM: 330
No hook, TI+JM: 175
No hook, JM+TM: 40
Chrome: 150 or 200 (seems bimodal)
Safari: 150-160
Opera: 3350
So I'll keep working on removing the hook, and we're much better without it, but it'd be awfully nice if TI+JM could do whatever it is JM+TM is doing!
Comment 5•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #2)
> Luckily, we seem to have a clazz->ext.isWrappedNative now!
Hah, I figured somebody else would eventually find that useful.
Assignee | ||
Comment 6•13 years ago
|
||
Well, I typed a long comment, but then Firefox decided to crash and lose it.
Brian, we end up taking stubcalls here because both times we enter compilation for that JSOP_EQ both rhs and lhs claim their type is unknown. So we never end up taking the fastpath for "JSOP_EQ on two objects" because we never know they're objects. Instead we take the intstring path with a null target, which looks like it sets up some guards we fail, and that's that.... I guess that's either a TI bug or a JM bug, but don't know which. Do you mind filing it as appropriate?
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #558028 -
Flags: review?(peterv)
Assignee | ||
Updated•13 years ago
|
Priority: -- → P1
Whiteboard: [need review]
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 8•13 years ago
|
||
I still need to look at what we are doing on this microbenchmark, but what should (in principle) be happening is that we know the lhs/rhs of the equality are objects, but because the property types of 'e' and 'document' are unknown (no type information for the DOM yet) we won't know statically whether they have equality hooks or not. Currently we only efficiently handle obj == obj cases when (a) the type is known, and we statically know there is no hook, and (b) nothing about the objects is known, and we generate an IC. There should be a (c) where the type is known but we need to dynamically test for an equality hook.
Comment 9•13 years ago
|
||
Comment on attachment 558028 [details] [diff] [review]
Add a way for an nsIXPCScriptable to request a null equality hook, and do so for DOM nodes.
Review of attachment 558028 [details] [diff] [review]:
-----------------------------------------------------------------
Please remove GetXPCWrappedNativeJSClassInfo too. It's already unused, but its result is wrong after this patch.
::: js/src/xpconnect/src/xpcpublic.h
@@ +75,5 @@
> nsresult
> xpc_MorphSlimWrapper(JSContext *cx, nsISupports *tomorph);
>
> extern JSBool
> XPC_WN_Equality(JSContext *cx, JSObject *obj, const jsval *v, JSBool *bp);
Can you remove this too?
::: js/src/xpconnect/src/xpcwrappednativejsops.cpp
@@ +1541,5 @@
> ops->typeOf = XPC_WN_JSOp_TypeOf_Object;
> }
>
> + if(mFlags.UseStubEqualityHook())
> + mJSClass.base.ext.equality = nsnull;
Should this assert that WantEquality returns false?
Attachment #558028 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 10•13 years ago
|
||
> Please remove GetXPCWrappedNativeJSClassInfo too.
Good catch. Removed function, revved nsIXPConnect iid.
> Can you remove this too?
Yes. Done.
> Should this assert that WantEquality returns false?
Yes. Done.
Assignee | ||
Updated•13 years ago
|
Whiteboard: [need review] → [need landing]
Comment 11•13 years ago
|
||
I am not familiar with XPC, but the naming "UseStubEqualityHook" is weird in some way. Maybe we could call that NoCustomEqualityHook ?
Assignee | ||
Comment 12•13 years ago
|
||
I modeled the name after UseJSStubForSetProperty.
The issue with NoCustomEqualityHook is that the "normal" (non-custom) equality hook XPConnect uses is what we want to avoid here.
Assignee | ||
Comment 13•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/754865ae97bc
I filed bug 685160 on the fact that we still get a stubcall here.
Comment 14•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•