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.

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

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

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

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.

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 on attachment 8561434 [details] [diff] [review] Patch Oops, restoring r=djvj

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

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

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

Thanks for all the reviews. https://hg.mozilla.org/integration/mozilla-inbound/rev/d96d552ff899

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

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.