Closed
Bug 870514
Opened 12 years ago
Closed 12 years ago
Shadowing listbase proxy own-property gets are slower than non-shadowing ones
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: bzbarsky, Assigned: djvj)
References
Details
Attachments
(4 files, 2 obsolete files)
919 bytes,
text/html
|
Details | |
6.09 KB,
patch
|
Details | Diff | Splinter Review | |
19.54 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
25.88 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
Consider the attached testcase. On it, I see numbers like this (ns/get):
Ion: 451
BC: 382
Interp: 458
if I implement [OverrideBuiltins] on DOMStringMap. This is slower than the numbers in bug 870508, except for interp which gets faster. This case _should_ be able to be faster, in general, because it never has to worry about the proto chain in this case...
![]() |
Reporter | |
Comment 1•12 years ago
|
||
Some profile data:
Ion: All the time is under GetPropertyIC::update. It breaks down like so:
38% DomListShadows (which has to perform a get under the hood to see if we shadow)
38% proxy_GetGeneric (does the actual get)
8% AutoFlushCache ctor
6% IonFrameIterator::safepoint
3% TypeMonitorResult
This seems like it could be improved with an IC for the shadowing case that just calls into the proxy....
BC: Almost all the time is under DoGetPropFallback, breaking down as:
43% EffectlesslyLookupProperty (calls DOMListShadows)
43% proxy_GetGeneric
3% TypeMonitorResult.
Looks like the same deal.
Interp only does the get on the proxy once, but spends a bunch of time doing TypeMonitorResult, propcache bits, etc. Still comes out ahead....
![]() |
Reporter | |
Comment 2•12 years ago
|
||
Ahead of Ion, that is.
Assignee | ||
Updated•12 years ago
|
Assignee: general → kvijayan
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #0)
> if I implement [OverrideBuiltins] on DOMStringMap.
Can I get the patch that implements this? I added a baseline stub for calling Proxy::get on shadowed properties, but I can't test it on this case without that change.
Assignee | ||
Comment 4•12 years ago
|
||
Patch for this case. Untested on the test case in the bug, but passes jit-tests and green on try (linux and linux64 only).
This patch only adds BaselineStubs for this case.
Assignee | ||
Comment 5•12 years ago
|
||
![]() |
Reporter | |
Comment 6•12 years ago
|
||
> Can I get the patch that implements this?
Sorry, I thought I'd uploaded it. Here it is!
![]() |
Reporter | |
Updated•12 years ago
|
Assignee: kvijayan → bzbarsky
![]() |
Reporter | |
Updated•12 years ago
|
Assignee: bzbarsky → kvijayan
![]() |
Reporter | |
Comment 7•12 years ago
|
||
For what it's worth, I tested the attached patch on the attached testcase in today's builds (which have some performance improvements on the DOM side compared to the builds from comment 0).
Without the patch the BC time is consistently about 235ns for me. With the patch it's consistently about 105ns.
A profile of the 105ns version shows a breakdown like so:
20% jitcode
70% under DOMStringMapBinding::DOMProxyHandler::get
1% js::ion::ProxyGet
9% js::Proxy::get
so we're doing much better, at first glance. I don't have very good visibility into the jitcode part, unfortunately...
Now all we need is an ion equivalent, right? ;)
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #7)
> For what it's worth, I tested the attached patch on the attached testcase in
> today's builds (which have some performance improvements on the DOM side
> compared to the builds from comment 0).
>
> Without the patch the BC time is consistently about 235ns for me. With the
> patch it's consistently about 105ns.
>
> A profile of the 105ns version shows a breakdown like so:
>
> 20% jitcode
> 70% under DOMStringMapBinding::DOMProxyHandler::get
> 1% js::ion::ProxyGet
> 9% js::Proxy::get
>
> so we're doing much better, at first glance. I don't have very good
> visibility into the jitcode part, unfortunately...
>
> Now all we need is an ion equivalent, right? ;)
Thanks for checking that boris. Been a bit occupied between work week stuff and another DOM bug I've been tracking down. The ion variant of this should be reasonably straightforward.
![]() |
Reporter | |
Comment 9•12 years ago
|
||
All good. Just saying that the general approach seems to be about right.
Assignee | ||
Comment 10•12 years ago
|
||
Ion version of the above.
Assignee | ||
Comment 11•12 years ago
|
||
Try run for baseline patch:
https://tbpl.mozilla.org/?tree=Try&rev=4ebb4d089f9d
Try run for ion patch (with baseline patch):
https://tbpl.mozilla.org/?tree=Try&rev=a8ec316af954
![]() |
Reporter | |
Comment 12•12 years ago
|
||
Without the two patches:
Ion: 263
BC: 230
Interp: 355
With the two patches:
Ion: 94
BC: 106
Interp: 357
(a few ns of noise in those numbers, but that's the general ballpark).
Pretty good; gets us down to close to the general competition ballpark. For reference:
Chrome:
Ion: 75
BC: 96
Interp: 118
Safari:
Ion: 92
BC: 126
Interp: 168
Assignee | ||
Updated•12 years ago
|
Attachment #748292 -
Flags: review?(hv1989)
Assignee | ||
Updated•12 years ago
|
Attachment #751096 -
Flags: review?(hv1989)
Comment 13•12 years ago
|
||
Comment on attachment 748292 [details] [diff] [review]
Untested patch for shadowing case Baseline stubs
Review of attachment 748292 [details] [diff] [review]:
-----------------------------------------------------------------
Nice! This is the first time reviewing DOM/ListBase. So I'm gonna be very cautious and list everything I'm doubting about.
- Why GetProp_GetListBaseShadowed and not GetProp_CallListBaseShadowed ?
- Shouldn't the JS_ASSERT in ICStubCompiler::guardProfilingEnabled get adjusted to include GetProp_GetListBaseShadowed?
::: js/src/ion/BaselineIC.cpp
@@ +3073,2 @@
> // For the remaining code, we need to reserve some registers to load a value.
> // This is ugly, but unvaoidable.
s/unvaoidable/unavoidable
Can you take this in this patch?
@@ +5963,5 @@
> + scratch = regs.takeAny();
> + regs.add(BaselineTailCallReg);
> + } else {
> + scratch = regs.takeAny();
> + }
What does this do and why? I think this should normally do:
GeneralRegisterSet regs(availableGeneralRegs(2));
Register scratch = regs.takeAny();
If there is a good enough reason, please add this in comments to the code.
Attachment #748292 -
Flags: review?(hv1989)
Comment 14•12 years ago
|
||
Comment on attachment 751096 [details] [diff] [review]
Ion ListBase shadowing stubs
Review of attachment 751096 [details] [diff] [review]:
-----------------------------------------------------------------
Nice!
::: js/src/ion/IonCaches.cpp
@@ +625,5 @@
>
> static void
> GenerateListBaseChecks(JSContext *cx, MacroAssembler &masm, JSObject *obj,
> + PropertyName *name, Register object, Label *stubFailure,
> + bool skipExpandoCheck=false)
spaces around "="
Attachment #751096 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 15•12 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #13)
> - Why GetProp_GetListBaseShadowed and not GetProp_CallListBaseShadowed ?
I think I'll actually remove the "Get" from "GetListBaseShadowed". Typically, I use the |Call| prefix for stubs which are calling into user defined callees (e.g. getters, setters of various sorts).
> - Shouldn't the JS_ASSERT in ICStubCompiler::guardProfilingEnabled get
> adjusted to include GetProp_GetListBaseShadowed?
Good catch. Yes.
> @@ +5963,5 @@
> > + scratch = regs.takeAny();
> > + regs.add(BaselineTailCallReg);
> > + } else {
> > + scratch = regs.takeAny();
> > + }
>
> What does this do and why? I think this should normally do:
>
> GeneralRegisterSet regs(availableGeneralRegs(2));
> Register scratch = regs.takeAny();
>
> If there is a good enough reason, please add this in comments to the code.
On x86 and x64, BaselineTailCallReg is not automatically reserved. In this case, we need a scratch reg, but it can't be BaselineTailCallReg because the scratch reg is used to push a stub frame, and that code doesn't allow for the scratch reg to be a BaselineTailCallReg. On platforms where BaslineTailCallReg can be part of the available register set, the conditional is used to ensure that scratch doesn't get allocated as that register.
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #748292 -
Attachment is obsolete: true
Attachment #752256 -
Flags: review?(hv1989)
Assignee | ||
Comment 17•12 years ago
|
||
Renamed the baseline stub from GetListBaseShadowed to just ListBaseShadowed. Added a helper routine (takeAnyPrecluding) to RegisterSet and changed stub code which allocates non-BaselineTailCall registers to use it.
Passes jit-tests.
Try run is: https://tbpl.mozilla.org/?tree=Try&rev=dd3da114bc35
Attachment #752256 -
Attachment is obsolete: true
Attachment #752256 -
Flags: review?(hv1989)
Attachment #752262 -
Flags: review?(hv1989)
Comment 18•12 years ago
|
||
Comment on attachment 752262 [details] [diff] [review]
Baseline ListBase Shadowing stubs v2
Review of attachment 752262 [details] [diff] [review]:
-----------------------------------------------------------------
Good to go ;)
Attachment #752262 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 19•12 years ago
|
||
Baseline Stub patch:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c4dd0489501e
Ion stub patch, followed by nit addressal (forgot to check fold into single patch before push):
http://hg.mozilla.org/integration/mozilla-inbound/rev/00af889e1634
http://hg.mozilla.org/integration/mozilla-inbound/rev/f4a05a051c99
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c4dd0489501e
https://hg.mozilla.org/mozilla-central/rev/00af889e1634
https://hg.mozilla.org/mozilla-central/rev/f4a05a051c99
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•