Last Comment Bug 684435 - Nix class equality hooks on DOM objects where possible
: Nix class equality hooks on DOM objects where possible
: perf
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz]
Depends on: 686434
Blocks: 683631
  Show dependency treegraph
Reported: 2011-09-02 19:03 PDT by Boris Zbarsky [:bz]
Modified: 2011-09-13 02:52 PDT (History)
13 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Microbenchmark (218 bytes, text/html)
2011-09-02 19:46 PDT, Boris Zbarsky [:bz]
no flags Details
Add a way for an nsIXPCScriptable to request a null equality hook, and do so for DOM nodes. (5.55 KB, patch)
2011-09-02 22:01 PDT, Boris Zbarsky [:bz]
peterv: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2011-09-02 19:03:08 PDT
Everything that has a unique XPCWN doesn't need one.
Comment 1 Boris Zbarsky [:bz] 2011-09-02 19:31:16 PDT
81 #define IS_WRAPPER_CLASS(clazz)                                               \
82     (clazz->ext.equality == js::Valueify(XPC_WN_Equality))

This will need to change.... :(
Comment 2 Boris Zbarsky [:bz] 2011-09-02 19:39:10 PDT
Luckily, we seem to have a clazz->ext.isWrappedNative now!
Comment 3 Boris Zbarsky [:bz] 2011-09-02 19:46:12 PDT
Created attachment 558022 [details]
Comment 4 Boris Zbarsky [:bz] 2011-09-02 19:53:45 PDT
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 Andrew McCreight [:mccr8] 2011-09-02 20:02:45 PDT
(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.
Comment 6 Boris Zbarsky [:bz] 2011-09-02 21:59:07 PDT
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?
Comment 7 Boris Zbarsky [:bz] 2011-09-02 22:01:10 PDT
Created attachment 558028 [details] [diff] [review]
Add a way for an nsIXPCScriptable to request a null equality hook, and do so for DOM nodes.
Comment 8 Brian Hackett (:bhackett) 2011-09-06 10:02:21 PDT
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 Peter Van der Beken [:peterv] 2011-09-07 06:34:19 PDT
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?
Comment 10 Boris Zbarsky [:bz] 2011-09-07 06:39:22 PDT
> 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.
Comment 11 Tom Schuster [:evilpie] 2011-09-07 07:04:33 PDT
I am not familiar with XPC, but the naming "UseStubEqualityHook" is weird in some way. Maybe we could call that NoCustomEqualityHook ?
Comment 12 Boris Zbarsky [:bz] 2011-09-07 07:08:45 PDT
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.
Comment 13 Boris Zbarsky [:bz] 2011-09-07 08:35:59 PDT

I filed bug 685160 on the fact that we still get a stubcall here.
Comment 14 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-08 10:24:37 PDT

Note You need to log in before you can comment on or make changes to this bug.