1129382 - Add Ion ICs for scripted getters/setters

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Baseline has ICs for this and we can optimize getters/setters in IonBuilder, but it'd be good to have Ion ICs as a fallback. deltablue.swf for instance has some sites where it accesses different getters and the IonBuilder path currently doesn't handle polymorphism.

Comment 1

[Nicolas B. Pierron [:nbp]]

Note: Assert that the Jit frames are properly aligned on JitStackAlignment.  This would avoid discovering fuzz bugs later.

Comment 2

[Jan de Mooij [:jandem]]

A WIP patch for getters is a pretty big win on deltablue.swf. With that + a fix for Shumway bug 1130397 we're 7x faster there (and faster than V8).

Comment 3

[Jan de Mooij [:jandem]]

Attachment: WIP

This works for getters. Still need to handle setters (should be very similar) and fix a few other things.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Adds Ion ICs for scripted getters/setters. It turned out to be pretty straight-forward, but I had to add a new JitFrame type and that required a bunch of changes everywhere.

Locally, on 32-bit, it improves our Shumway deltablue.swf score from 2200 to 6900 points. V8 gets 5900 points, but my V8 build is outdated so it's possible they are faster. In any case, it's a pretty big win.

Flagging efaust for the IonCaches.cpp changes, djvj for profiler parts, nbp for the various JitFrame* files.

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 8561434 [details] [diff] [review]
Patch

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

OK this looks OK to me. I would appreciate if Nicolas also glanced at the frame building to ensure that we are setting up ion the way it expects.

::: js/src/jit/IonCaches.cpp
@@ +990,5 @@
>          masm.adjustStack(IonOOLNativeExitFrameLayout::Size(0));
> +    } else if (IsCacheableGetPropCallPropertyOp(obj, holder, shape)) {
> +        Register argJSContextReg = regSet.takeGeneral();
> +        Register argUintNReg     = regSet.takeGeneral();
> +        Register argVpReg        = regSet.takeGeneral();

Sure, this duplication is maybe easier for future readers. No change required.

@@ +2434,5 @@
> +        masm.reserveStack(padding);
> +
> +        if (target->nargs() > 1) {
> +            for (size_t i = 0; i < target->nargs() - 1; i++)
> +                masm.Push(UndefinedValue());

for (size_t i = 1; i < target->nargs(); i++) ? Admittedly, this is maybe "more sneaky" for the eye.

Comment 6

[Jan de Mooij [:jandem]]

Oh just realized this patch needs an extra check to be safe with the innerize-window optimization (I missed the comment in CanAttachNativeGetProp). Posting that here so I don't forget about it...

Comment 7

[Kannan Vijayan [:djvj]]

Comment on attachment 8561434 [details] [diff] [review]
Patch

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

Just reviewed the profiling instrumentation code, due to Jan's comment.  Looks good.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8561434 [details] [diff] [review]
Patch

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

Sorry for the late review.
This patch looks good :)

::: js/src/jit-test/tests/ion/scripted-getter-setter.js
@@ +22,5 @@
> +    }
> +    function setter(a, b) {
> +	assertEq(arguments.length, 1);
> +	assertEq(b, undefined);
> +	assertJitStackInvariants();

Can you add a bailout() call after assertJitStackInvariant, such that as soon as the getter/setter is Ion compiled we can check if we can successfully bail out.

@@ +34,5 @@
> +}
> +function f() {
> +    var objs = getObjects();
> +    var res = 0;
> +    for (var i=0; i<2000; i++) {

can you use setJitCompilerOption to reduce ion threshold to something like 50 ?

::: js/src/jit/IonCaches.cpp
@@ +1052,5 @@
> +        // The JitFrameLayout pushed below will be aligned to JitStackAlignment,
> +        // so we just have to make sure the stack is aligned after we push the
> +        // |this| + argument Values.
> +        uint32_t argSize = (target->nargs() + 1) * sizeof(Value);
> +        uint32_t padding = ComputeByteAlignment(masm.framePushed() + argSize, JitStackAlignment);

nice :)

@@ +2425,5 @@
> +
> +        // The JitFrameLayout pushed below will be aligned to JitStackAlignment,
> +        // so we just have to make sure the stack is aligned after we push the
> +        // |this| + argument Values.
> +        uint32_t numArgs = Max(size_t(1), target->nargs());

nit: Add a test case where the setter has no formal arguments.

::: js/src/jit/JitFrameIterator.h
@@ +64,5 @@
>      // information is recorded on the JitActivation.
> +    JitFrame_Bailout,
> +
> +    // Ion IC calling a scripted getter/setter.
> +    JitFrame_IonAccessorIC

JitFrame_Exit and JitFrame_Bailout  are not encoded on the stack, move this definition before all the Unwound frames.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8561434 [details] [diff] [review]
Patch

Oops, restoring r=djvj

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> can you use setJitCompilerOption to reduce ion threshold to something like
> 50 ?

I don't really like that option because it makes running jit-tests with --ion-eager or --ion-warmup-threshold=X less effective (it'll increase the threshold from 0 to 50 I think).

It'd be nice to change the option so that it only sets the threshold if the new value < the current one. What do you think? Maybe we could have two options if we need the current behavior somewhere.

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> (In reply to Nicolas B. Pierron [:nbp] from comment #8)
> > can you use setJitCompilerOption to reduce ion threshold to something like
> > 50 ?
> 
> I don't really like that option because it makes running jit-tests with
> --ion-eager or --ion-warmup-threshold=X less effective (it'll increase the
> threshold from 0 to 50 I think).
> 
> It'd be nice to change the option so that it only sets the threshold if the
> new value < the current one. What do you think? Maybe we could have two
> options if we need the current behavior somewhere.

I don't think that a good idea for fuzzers, as they would always be fuzzing with Ion eager if we do so.
On the other hand we can no-op this if --ion-eager is used.

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> I don't think that a good idea for fuzzers, as they would always be fuzzing
> with Ion eager if we do so.

Why? The fuzzers use --ion-eager but not always. They use a ton of different flag combinations.

> On the other hand we can no-op this if --ion-eager is used.

Yes that'd already help. For now I'm changing the test to only set the threshold to 50 if the current value > 50.

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #12)
> (In reply to Nicolas B. Pierron [:nbp] from comment #11)
> > I don't think that a good idea for fuzzers, as they would always be fuzzing
> > with Ion eager if we do so.
> 
> Why? The fuzzers use --ion-eager but not always. They use a ton of different
> flag combinations.

Because this implies that the setJitCompilerOption cannot reduce the threshold, and thus after calling this function multiple times with random values (which I expected the fuzzers to be doing), will end-up doing the equivalent of --ion-eager, even when it is not used on the command line.

Comment 14

[Jan de Mooij [:jandem]]

Thanks for all the reviews.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d96d552ff899

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #13)
> Because this implies that the setJitCompilerOption cannot reduce the
> threshold, and thus after calling this function multiple times with random
> values (which I expected the fuzzers to be doing), will end-up doing the
> equivalent of --ion-eager, even when it is not used on the command line.

Maybe we can do this: *if* you use a command line flag like --ion-eager or --ion-warmup-threshold=X, setJitCompilerOption is a no-op when the argument is > this *original* value.

Then if we run a test with --ion-eager, setJitCompilerOption does not increase the threshold. If we run the test with no flags, setJitCompilerOption(.., 50) will set the threshold to 50. If the fuzzers don't use these shell flags, they can still call setJitCompilerOption with whatever value they want.

So the shell flags, if present, would override setJitCompilerOption.

Comment 16

[Jan de Mooij [:jandem]]

Thinking about it more, making it a no-op when --ion-eager is used is probably the simplest option for now, I think we should do that.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d96d552ff899