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


Attachments
Microbenchmark (218 bytes, text/html)
2011-09-02 19:46 PDT, Boris Zbarsky [:bz] (still a bit busy)
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] (still a bit busy)
peterv: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2011-09-02 19:03:08 PDT
Everything that has a unique XPCWN doesn't need one.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 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] (still a bit busy) 2011-09-02 19:39:10 PDT
Luckily, we seem to have a clazz->ext.isWrappedNative now!
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-09-02 19:46:12 PDT
Created attachment 558022 [details]
Microbenchmark
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 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] (still a bit busy) 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] (still a bit busy) 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] (still a bit busy) 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 AWAY 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] (still a bit busy) 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] (still a bit busy) 2011-09-07 08:35:59 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/754865ae97bc

I filed bug 685160 on the fact that we still get a stubcall here.

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