The default bug view has changed. See this FAQ.

Speed up js::IsWrapper

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Summary: Calls to js::IsWrapper from XPConnect are slow. → Calls to js::IsWrapper from XPConnect are slow
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.
Created attachment 597575 [details] [diff] [review]
part 1 - Don't do a redundant class lookup in js::IsProxy. v1
Attachment #597575 - Flags: review?(luke)
Created attachment 597576 [details] [diff] [review]
part 2 - Make js::IsWrapper fully inline-able. v1
Attachment #597576 - Flags: review?(luke)
Summary: Calls to js::IsWrapper from XPConnect are slow → Speed up js::IsWrapper

Comment 4

5 years ago
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.
Attachment #597575 - Flags: review?(luke) → review+

Comment 5

5 years ago
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 ;-)
Attachment #597576 - Flags: review?(luke) → review+
(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.
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
Pushed to m-i: 

http://hg.mozilla.org/integration/mozilla-inbound/rev/9795ad013aa9
http://hg.mozilla.org/integration/mozilla-inbound/rev/83dfb2a53719
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/83dfb2a53719
https://hg.mozilla.org/mozilla-central/rev/9795ad013aa9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.