1266092 - Consider removing MGetPropertyCache and instead use the shared stub

Status: RESOLVED INVALID

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

For jsop_getprop we currently have two caches. The ion caches and the shared stub caches (baseline caches).

The current state is that we will try to use the ion caches first and if that fails only implement the shared stub caches.
It was reasoned that the ion caches would be faster, since they are tailored for ionmonkey.
Though one of the issues is that they implement less functionality. The shared stubs are more complete.

I just ran the numbers and found the following:

With ion JSOP_GETPROP stubs
> $ JIT_OPTION_forceInlineCaches=true js run.js
> Richards: 5023
> DeltaBlue: 5931
> Crypto: 1263
> RayTrace: 11470
> EarleyBoyer: 10140
> RegExp: 1407
> Splay: 2548
> SplayLatency: 9810
> NavierStokes: 1099
> PdfJS: 5651
> Mandreel: 1628
> MandreelLatency: 6719
> Gameboy: 10289
> CodeLoad: 11938
> Box2D: 5876
> zlib: 67231
> Typescript: 14264
> ----
> Score (version 9): 5708

When ion JSOP_GETPROP stubs are removed (i.e. using shared stubs)
> $ JIT_OPTION_forceInlineCaches=true js run.js
> Richards: 5722
> DeltaBlue: 6327
> Crypto: 1246
> RayTrace: 11645
> EarleyBoyer: 10083
> RegExp: 1374
> Splay: 2627
> SplayLatency: 9986
> NavierStokes: 1092
> PdfJS: 5870
> Mandreel: 1604
> MandreelLatency: 5684
> Gameboy: 10411
> CodeLoad: 12598
> Box2D: 6168
> zlib: 66702
> Typescript: 14038
> ----
> Score (version 9): 5760


So it seems like an improvement on octane? Should we look to remove the ion JSOP_GETPROP cache?

Comment 1

[Eric Faust [:efaust]]

I think that we will find that bz is unhappy if we do this. The baseline support for DOM proxies is incomplete, relative to the work in the IonCaches. As long as this does not regress, I am happy to do this.

Comment 2

[Hannes Verschore [:h4writer]]

(In reply to Eric Faust [:efaust] from comment #1)
> I think that we will find that bz is unhappy if we do this. The baseline
> support for DOM proxies is incomplete, relative to the work in the
> IonCaches. As long as this does not regress, I am happy to do this.

Would it be enough to test dromaeo to have some numbers? And do you have an idea offhand which DOM proxies are not in the baseline stubs, but are in the ion stubs.

Comment 3

[Jan de Mooij [:jandem]]

I wonder if the Ion ICs have a performance issue somewhere - there's no reason for them to be slower right? At least on Octane, both Baseline and Ion stubs should be able to handle most/all cases. How does it compare without forceInlineCaches=true? 

The Ion GetProp IC can be marked as idempotent - that was a win when we added it, but maybe it doesn't help much these days?

The CacheIR work would also address this btw without falling back to a worse IC, hopefully I can get back to that work soon, after dealing with fuzz bugs and top crashes.

Comment 4

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> I wonder if the Ion ICs have a performance issue somewhere - there's no
> reason for them to be slower right? At least on Octane, both Baseline and
> Ion stubs should be able to handle most/all cases. How does it compare
> without forceInlineCaches=true? 

Gonna do that tomorrow. Though I think we don't use a lot of caches in octane when not forced.

> The Ion GetProp IC can be marked as idempotent - that was a win when we
> added it, but maybe it doesn't help much these days?
> 
> The CacheIR work would also address this btw without falling back to a worse
> IC, hopefully I can get back to that work soon, after dealing with fuzz bugs
> and top crashes.

I was also suprised it was faster. All common sense would say ion caches are faster.
Now the CacheIR will definitely remove ion GetProp IC and take precedence on shared stubs.
But if the shared caches are currently faster, I wouldn't mind to remove the ion GetProp ICs already
before CacheIR is ready to take over. I think we shouldn't investigate possible GetProp IC issues because it will get removed ...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Would it be enough to test dromaeo to have some numbers?

I don't know.  You'd want to see whether we end up with the relevant MGetPropertyCache codepath on those dromaeo bits.

But in general, I suspect that most dromaeo stuff ends up taking the MGetDOMProperty path instead.  We tried pretty hard for that.

> And do you have an idea offhand which DOM proxies are not in the baseline stubs

I think all the DOM proxy cases in ion for the getproperty cache would end up going through GetPropertyIC::tryAttachProxy.  This does what I'd expect: does the shadowing check, and then calls either tryAttachDOMProxyShadowed or tryAttachDOMProxyUnshadowed.

tryAttachDOMProxyShadowed just emits a call to Proxy::get.  

tryAttachDOMProxyUnshadowed does some more interesting stuff: guards on the expando not shadowing (including the expandoandgeneration check as needed), then outputs a LoadSlot or GetterCall, depending on what the shape from the lookup looks like.

---------------------

OK, so now baseline.  In baseline, things are a lot more complicated.  :(  

We can start off in TryAttachNativeGetAccessorPropStub.  If we're shadowing, we call Proxy::get.  If not, we output a GetProp_CallDOMProxyWithGenerationNative or GetProp_CallDOMProxyNative (once you untangle the logic).  So that's all fine.

What I'm not seeing is anything to handle the "get on a proxy, find the thing in a slot somewhere up the proto chain" case.  Or for that matter reading from slots for normal (not unboxed, not typed) objects at all.  Am I just missing it?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and if I'm right, then forcing ICs and doing some document.getElementsByTagName("foo") calls should probably show a difference: that's a slot get on the proto.

Comment 7

[Jan de Mooij [:jandem]]

It'd also be interesting to look at some micro-benchmarks, to see what the shared stub overhead is.

On most of the JS benchmarks ICs are not a performance bottleneck, but I'm a bit worried about normal websites and frameworks, where ICs can be hot and our performance there isn't that great to begin with.

Comment 8

[Jan de Mooij [:jandem]]

Another thing to consider is that MGetPropertyCache is also used for GETELEM..

Comment 9

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> Another thing to consider is that MGetPropertyCache is also used for
> GETELEM..

Hmmm. I also got some bad news. Did some micro benchmarks and I noticed a huge decrease. Like predicted the baseline caches are quite slow: 4800ms vs 3200ms. Which let me wonder why octane was faster. I remeasured it and found the opposite to be true. Octane was faster with ion caches. Gonna retry later again, but for now it seems we shouldn't remove the MGetPropertyCaches!

Comment 10

[Hannes Verschore [:h4writer]]

Because the overhead of shared stubs this isn't a viable option. The Ion caches are definitely faster after extra investigation. Therefore marking this as an invalid bug.