Closed Bug 1310125 Opened 3 years ago Closed 3 years ago

Port Baseline scripted getter stub to CacheIR

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

I've been putting this off for a while because Baseline's getter stubs are pretty complicated atm and we need some new CacheIR infrastructure (this is the first stub that can make GC calls etc).
Baseline getter/setter ICs always store the |holder| JSObject. For own properties, this doesn't make a lot of sense as there could be many different objects with the same shape.

This patch changes BaselineInspector to return |holder == nullptr| if we have an own property getter/setter.
Attachment #8801028 - Flags: review?(hv1989)
Sorry, this is a large patch, because it does some refactoring:

* CacheIR stubs are now keyed on the |engine|, like non-CacheIR stubs. This was not necessary until now as all stubs had exactly the same code in Ion/Baseline mode.

* CacheIR stubs now store a makesGCCalls bit in the CacheIRStubInfo structure (shared across similar stubs). For Non-CacheIR stubs, this is based on the stub kind but that doesn't work for CacheIR stubs as they all use the same Kind. This is one other thing that we no longer have to track explicitly, as we set this bit automatically when the stub uses a stub frame.

* For BaselineDebugModeOSR, we have to implement the Clone() method for CacheIR stubs. Again, after this it should be handled automatically for all stubs without requiring more boilerplate.

* The CacheIR pattern matching in BaselineInspector is a bit annoying, but I made sure it matches all cases correctly. Having a generic CacheIR to MIR compiler might let us get rid of this code.
Attachment #8801033 - Flags: review?(hv1989)
One problem is that I'm not sure how to do something similar to UpdateExistingGetPropCallStubs for CacheIR stubs. If it finds a similar stub with a Shape that no longer matches, it updates it instead of attaching a new stub.

I think this was mostly done to help Ion's BaselineInspector code, so for getters on the proto, I changed BaselineInspector to skip stubs that have |holder->shape() != holderShape|. This should at least keep us from using "outdated" stubs in that case.

I hope this will be good enough. If not, we can add some mechanism to check previous CacheIR stubs: for instance, if we see a previous stub with the same CacheIR that has a LoadObject -> GuardShape and the shape is known to no longer match, we can reuse it. This optimization would then work automatically for *all* stubs (also for stubs for plain data properties etc). This should be pretty easy to implement - we mostly have to change CacheIRReader to make it easier to skip uninteresting ops.
> I think this was mostly done to help Ion's BaselineInspector code

That's correct.

> I changed BaselineInspector to skip stubs that have |holder->shape() != holderShape|

That seems like it would be ok, yes.  It's worth retesting on the testcases in the actual bugs that added the UpdateExisting* things.
Comment on attachment 8801028 [details] [diff] [review]
Part 1 - Don't use |holder| for own property getters/setters in IonBuilder

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

Not too familiar with how this all works. So learning while doing the reviews as we need more/new people understanding this better.
But this looks good to me.
Attachment #8801028 - Flags: review?(hv1989) → review+
Comment on attachment 8801033 [details] [diff] [review]
Part 2 - Port scripted getter stub to CacheIR

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

There a lot of parts/aspects to this patch. Therefore I'm gonna reread a few times with different emphasis and posting my comments in between to not loose them.
Overall it looks good.

It is quite ironic to have the BaselineCacheIRCompiler and IonCacheIRCompiler (in the future) and BaselineCache, which has an "engine" and can emit code for IonMonkey using SharedICs.
But I assume that is the status quo until we have the IonCacheIRCompiler.

Not related or introduce in this bug, but how does "isTemporarilyUnoptimizable" work?

::: js/src/jit/BaselineInspector.cpp
@@ +722,5 @@
> +    // or:
> +    //
> +    //   GuardGroup objId
> +    //   expandoId: GuardAndLoadUnboxedExpando
> +    //   GuardShape expandoId

+1 for these comments. They help a lot to quickly glance what happens here.
Can we add them also to?
- TestMatchingReceiver
- EmitReadSlotResult
- tryAttachXXX functions?

(follow-up bug is fine)

@@ +752,5 @@
> +    return true;
> +}
> +
> +static bool
> +AddCacheIRGetPropFunction(ICCacheIR_Monitored* stub, JSObject** holder, Shape** holderShape,

maybe name it IsAddCacheIRGetPropFunction ?

::: js/src/jit/CacheIR.cpp
@@ +251,5 @@
> +        ObjOperandId holderId = writer.loadObject(holder);
> +        writer.guardShape(holderId, holder->as<NativeObject>().lastProperty());
> +    }
> +
> +    if (IsCacheableGetPropCallScripted(obj, holder, shape)) {

What about inverting the if statement. That way the main code can intended 4 spaces less.

if (!IsCacheable....)
   MOZ_CRASH();

...
Comment on attachment 8801033 [details] [diff] [review]
Part 2 - Port scripted getter stub to CacheIR

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

::: js/src/jit/BaselineCacheIR.cpp
@@ +233,5 @@
>      Register reg_;
>  
>    public:
> +    AutoScratchRegister(CacheRegisterAllocator& alloc, MacroAssembler& masm,
> +                        Register excluding = InvalidReg)

e.g. AutoScratchRegister(alloc, masm, eax);

This looks too much like we just took eax as scratch register.
Shouldn't we make the default behaviour to take that register.
Like AutoFixedScratchRegister does?

We can rename this variant to AutoScratRegisterExclude(alloc, masm, eax); to signal
the reader that this will not take eax.

s/AutoScratchRegister/AutoScratchRegisterExclude/
s/AutoScratchRegisterFixed/AutoScratchRegister/

and give AutoScratchRegister the default InvalidReg instead of AutoScratchRegisterExclude.

@@ +427,5 @@
>  
>  // BaselineCacheIRCompiler compiles CacheIR to BaselineIC native code.
>  class MOZ_RAII BaselineCacheIRCompiler : public CacheIRCompiler
>  {
> +    ICStubEngine engine_;

I think this needs a comment. Something like:

Some baseline ICStubs in can get compiled in IonMonkey through SharedStubs.
Those stubs have different machine code as a result we need to keep track
if we are compiling for Baseline or IonMonkey.

@@ +820,5 @@
> +            availableRegs_.add(loc.valueReg());
> +            availableRegs_.take(reg);
> +            currentOpRegs_.add(reg);
> +            return;
> +        }

As follow-up bug, should we first try to get another reg and spill to that register before resorting to pushing to stack? It is also not optimal since we could be moving it multiple times, but we don't use that many fixed registers (2 max?) instead of pushing to stack and almost immediately getting it back?
Comment on attachment 8801033 [details] [diff] [review]
Part 2 - Port scripted getter stub to CacheIR

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

Looks good!
Attachment #8801033 - Flags: review?(hv1989) → review+
(I'll land this after the merge next week.)
Priority: P3 → P2
(In reply to Hannes Verschore [:h4writer] from comment #6)
> Not related or introduce in this bug, but how does
> "isTemporarilyUnoptimizable" work?

There's a flag on some Baseline fallback stubs that's set when we have an unoptimizable case. Ion code looking at Baseline stubs then knows that there were other cases besides the ones for which we attached stubs. Sometimes we don't want to set that flag even if we didn't attach a stub, for instance when we have a scipted getter that has no JIT code yet. In that case we set isTemporarilyUnoptimizable.

> ::: js/src/jit/BaselineInspector.cpp
> +1 for these comments. They help a lot to quickly glance what happens here.
> Can we add them also to?

Not sure it's worth it myself, but I'll file a follow-up bug.

> What about inverting the if statement. That way the main code can intended 4
> spaces less.
> 
> if (!IsCacheable....)
>    MOZ_CRASH();

It makes it harder to add new cases in the future. I changed it to a MOZ_ASSERT and removed the if-else.

> s/AutoScratchRegister/AutoScratchRegisterExclude/
> s/AutoScratchRegisterFixed/AutoScratchRegister/
> 
> and give AutoScratchRegister the default InvalidReg instead of
> AutoScratchRegisterExclude.

Excellent suggestion! This is indeed much nicer. I changed it to AutoScratchRegisterExcluding, for consistency with RegisterSet::takeAnyExcluding.
Priority: P2 → P1
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c898900950
part 1 - Don't use |holder| for own property getters/setters in IonBuilder. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/3656a6f2cd7e
part 2 - Port Baseline scripted getter IC stub to CacheIR. r=h4writer
https://hg.mozilla.org/mozilla-central/rev/a9c898900950
https://hg.mozilla.org/mozilla-central/rev/3656a6f2cd7e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(In reply to Guilherme Lima from comment #12)
> These patches appear to have:
> regressed Octane-GameBoy by 21%
> (https://arewefastyet.com/
> #machine=29&view=single&suite=octane&subtest=Gameboy)
> regressed misc-bugs-608733-interpreter by 95%
> (https://arewefastyet.com/#machine=29&view=single&suite=misc&subtest=bugs-
> 608733-interpreter)
> regressed misc-f32-fft by 9%
> (https://arewefastyet.com/#machine=29&view=single&suite=misc&subtest=f32-fft)
> 
> and improved misc-typedobj-splay-standard by 7%
> (https://arewefastyet.com/
> #machine=29&view=single&suite=misc&subtest=typedobj-splay-standard)

Thanks for the report!

@Jan: haven't opened a new bug report yet. Could be a small oversight and quick fix. If you prefer I can open a new bug report for you.
Flags: needinfo?(jdemooij)
I added the isTemporarilyUnoptimizable argument to IsCacheableGetPropCallScripted, but we only want to set that to true if we have a scripted getter.

This fixes the misc-interpreter regression locally.
Flags: needinfo?(jdemooij)
Attachment #8811653 - Flags: review?(hv1989)
Comment on attachment 8811653 [details] [diff] [review]
Part 3 - Fix regression

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

Thanks for the quick look and fix!
Attachment #8811653 - Flags: review?(hv1989) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68b33d652efc
part 3 - Fix IsCacheableGetPropCallScripted perf regression. r=h4writer
This patch fixed all the changes the previous ones have caused =)
Depends on: 1318634
Just to confirm. I see all shifts in benchmarks have gone to the normal speed:
https://treeherder.mozilla.org/perf.html#/alerts?id=4204
You need to log in before you can comment on or make changes to this bug.