Add Ion ICs for scripted getters/setters

RESOLVED FIXED in Firefox 38

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
Note: Assert that the Jit frames are properly aligned on JitStackAlignment.  This would avoid discovering fuzz bugs later.
(Assignee)

Comment 2

4 years ago
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).
(Assignee)

Comment 3

4 years ago
Created attachment 8560534 [details] [diff] [review]
WIP

This works for getters. Still need to handle setters (should be very similar) and fix a few other things.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Created attachment 8561434 [details] [diff] [review]
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.
Attachment #8560534 - Attachment is obsolete: true
Attachment #8561434 - Flags: review?(nicolas.b.pierron)
Attachment #8561434 - Flags: review?(kvijayan)
Attachment #8561434 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Blocks: 1128646

Comment 5

4 years ago
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.
Attachment #8561434 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 6

4 years ago
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 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.
Attachment #8561434 - Flags: review?(kvijayan) → review+
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.
Attachment #8561434 - Flags: review?(nicolas.b.pierron)
Attachment #8561434 - Flags: review?(kvijayan)
Attachment #8561434 - Flags: review+
Comment on attachment 8561434 [details] [diff] [review]
Patch

Oops, restoring r=djvj
Attachment #8561434 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 10

4 years ago
(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.
Flags: needinfo?(nicolas.b.pierron)
(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.
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 12

4 years ago
(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.
(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.
(Assignee)

Comment 15

4 years ago
(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.
(Assignee)

Comment 16

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/d96d552ff899
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1132584
(Assignee)

Updated

4 years ago
Blocks: 626021
(Assignee)

Updated

4 years ago
Depends on: 1132770
Duplicate of this bug: 965110
You need to log in before you can comment on or make changes to this bug.