Closed Bug 684435 Opened 11 years ago Closed 11 years ago

Nix class equality hooks on DOM objects where possible


(Core :: XPConnect, defect, P1)






(Reporter: bzbarsky, Assigned: bzbarsky)



(Keywords: perf)


(2 files)

Everything that has a unique XPCWN doesn't need one.
Blocks: 683631
81 #define IS_WRAPPER_CLASS(clazz)                                               \
82     (clazz->ext.equality == js::Valueify(XPC_WN_Equality))

This will need to change.... :(
Luckily, we seem to have a clazz->ext.isWrappedNative now!
Attached file Microbenchmark
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!
(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.
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?
Priority: -- → P1
Whiteboard: [need review]
OS: Mac OS X → All
Hardware: x86 → All
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 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+
> 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.
Whiteboard: [need review] → [need landing]
I am not familiar with XPC, but the naming "UseStubEqualityHook" is weird in some way. Maybe we could call that NoCustomEqualityHook ?
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.

I filed bug 685160 on the fact that we still get a stubcall here.
Flags: in-testsuite?
Keywords: perf
Whiteboard: [need landing]
Target Milestone: --- → mozilla9
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 686434
You need to log in before you can comment on or make changes to this bug.