Boris Zbarsky [:bzbarsky] Reporter
	Description • 11 years ago

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

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 1 • 11 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....

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 2 • 11 years ago

Ahead of Ion, that is.

	Kannan Vijayan [:djvj] Assignee
	Comment 3 • 11 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.

	Kannan Vijayan [:djvj] Assignee
	Comment 4 • 11 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.

	Kannan Vijayan [:djvj] Assignee
	Comment 5 • 11 years ago

Try run: https://tbpl.mozilla.org/?tree=Try&rev=8c7832039b4b

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 6 • 11 years ago

> Can I get the patch that implements this?

Sorry, I thought I'd uploaded it.  Here it is!

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 7 • 11 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?  ;)

	Kannan Vijayan [:djvj] Assignee
	Comment 8 • 11 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.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 9 • 11 years ago

All good.  Just saying that the general approach seems to be about right.

	Kannan Vijayan [:djvj] Assignee
	Comment 10 • 11 years ago

Ion version of the above.

	Kannan Vijayan [:djvj] Assignee
	Comment 11 • 11 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

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 12 • 11 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

	Hannes Verschore [:h4writer]
	Comment 13 • 11 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.

	Hannes Verschore [:h4writer]
	Comment 14 • 11 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 "="

	Kannan Vijayan [:djvj] Assignee
	Comment 15 • 11 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.

	Kannan Vijayan [:djvj] Assignee
	Comment 17 • 11 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

	Hannes Verschore [:h4writer]
	Comment 18 • 11 years ago

Comment on attachment 752262 [details] [diff] [review]
Baseline ListBase Shadowing stubs v2

Review of attachment 752262 [details] [diff] [review]:
-----------------------------------------------------------------

Good to go ;)

	Kannan Vijayan [:djvj] Assignee
	Comment 19 • 11 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

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 11 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