717113 - Speed up js::IsWrapper

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

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)]

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)]

Attachment: part 1 - Don't do a redundant class lookup in js::IsProxy. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: part 2 - Make js::IsWrapper fully inline-able. v1

Comment 4

[Luke Wagner [:luke]]

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]]

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)]

(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.

Comment 7

[Bobby Holley (:bholley)]

I pushed a baseline to try here: https://tbpl.mozilla.org/?tree=Try&rev=eb23b108438f

and the patches to try here: https://tbpl.mozilla.org/?tree=Try&rev=f5c817d4529c

This seems like a modest win in general, so I'm going to push it: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=eb23b108438f&newRev=f5c817d4529c&tests=dromaeo_basics,dromaeo_css,dromaeo_dom,dromaeo_jslib,dromaeo_sunspider,dromaeo_v8,tsspider,tsspider_nochrome,tsspider_paint,tsspider_nochrome_paint,v8&submit=true

Comment 8

[Bobby Holley (:bholley)]

Pushed to m-i: 

http://hg.mozilla.org/integration/mozilla-inbound/rev/9795ad013aa9
http://hg.mozilla.org/integration/mozilla-inbound/rev/83dfb2a53719

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/83dfb2a53719
https://hg.mozilla.org/mozilla-central/rev/9795ad013aa9