Last Comment Bug 717113 - Speed up js::IsWrapper
: Speed up js::IsWrapper
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-10 17:18 PST by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-02-22 10:45 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - Don't do a redundant class lookup in js::IsProxy. v1 (1.90 KB, patch)
2012-02-15 14:57 PST, Bobby Holley (:bholley) (busy with Stylo)
luke: review+
Details | Diff | Splinter Review
part 2 - Make js::IsWrapper fully inline-able. v1 (2.58 KB, patch)
2012-02-15 14:57 PST, Bobby Holley (:bholley) (busy with Stylo)
luke: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-01-10 17:18:16 PST
This caused bug 622301 to regress dromaeo. We can be a bit smarter in that patch about calling js::IsWrapper, but there's also some speedup to be gained in js-land.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-02-15 14:56:50 PST
So, the issue in bug 622301 turned out to be that I was putting some stuff on the critical path that didn't need to be. So this stuff is no longer quite so critical, but I still think it's probably worth making this fast, especially given the new dom bindings. So I'm attaching the patches and flagging luke for review.
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-02-15 14:57:09 PST
Created attachment 597575 [details] [diff] [review]
part 1 - Don't do a redundant class lookup in js::IsProxy. v1
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-02-15 14:57:24 PST
Created attachment 597576 [details] [diff] [review]
part 2 - Make js::IsWrapper fully inline-able. v1
Comment 4 Luke Wagner [:luke] 2012-02-20 11:34:17 PST
Comment on attachment 597575 [details] [diff] [review]
part 1 - Don't do a redundant class lookup in js::IsProxy. v1

>+    Class *clasp = GetObjectClass(obj);
>+    return IsObjectProxyClass(clasp);

Feel free to turn this and the next one into one-liners.
Comment 5 Luke Wagner [:luke] 2012-02-20 11:45:45 PST
Comment on attachment 597576 [details] [diff] [review]
part 2 - Make js::IsWrapper fully inline-able. v1

>-static int sWrapperFamily;
>+namespace js {
>+int WrapperFamily;
>+}

I think 's' is the prefix for static data; can you keep it sWrapperFamily?

>+extern JS_FRIEND_DATA(int) WrapperFamily;
>+
>+inline bool
>+IsWrapper(const JSObject *obj)
>+{
>+    return IsProxy(obj) && GetProxyHandler(obj)->family() == &WrapperFamily;
>+}

Are there any measurable speedups?  If so, there may be a little extra fat to trim: I believe computing &WrapperFamily requires a load or two due to PIC.  We could be dirty and use a magic constant to shave off those loads.  (Oooh, 'xpc' in 1337-speak is 0x9c ;-)
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-02-20 20:33:50 PST
(In reply to Luke Wagner [:luke] from comment #4)
> Feel free to turn this and the next one into one-liners.

Done.


(In reply to Luke Wagner [:luke] from comment #5)
> I think 's' is the prefix for static data; can you keep it sWrapperFamily?

Oh, I see. I thought it implied static linkage, which is no longer the case. Changed it back.

> Are there any measurable speedups?  If so, there may be a little extra fat
> to trim: I believe computing &WrapperFamily requires a load or two due to
> PIC.  We could be dirty and use a magic constant to shave off those loads. 
> (Oooh, 'xpc' in 1337-speak is 0x9c ;-)

I think I'll hold off on this one for now until we see the effect of this patch. If the results are significant, this sounds like a good plan.

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