This is much like bug 649887 but for setelem or setprop.

In pursuing proxy perfection, we need to make the SetProp ICs accessible in places where TI is unsure about the incoming types (since proxies don't keep TI, this shouldn't cause extra invalidations). In working on that (patch to come shortly), I discovered that our SetProperty ICs were broken! Here's a fix :)

Comment on attachment 792664 [details] [diff] [review] Part -1: Fix frightening SetProperty() call Canceling review. This was found and fixed independently in bug 906284.

looking fairly happy on try: https://tbpl.mozilla.org/?tree=Try&rev=5a73ea8e5e9e patches up in the morning

In order to get proxies into the ICs, we need to teach them how to deal with objects with less-than-completely understood objects flowing in.

Both proxy::get and proxy::set have the same marking profile, so we can reuse the fake exit frame.

Comment on attachment 793007 [details] [diff] [review] Part 0: Open SetPropertyIC to cases with uncertain TI. Review of attachment 793007 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonCaches.cpp @@ +1948,5 @@ > > + Label failures; > + masm.branchPtr(Assembler::NotEqual, > + Address(object(), JSObject::offsetOfShape()), > + ImmGCPtr(obj->lastProperty()), &failures); In the needsTypeBarrier() case, this needs a type test as well as a shape test. Two objects with the same shape can have different types. @@ +1950,5 @@ > + masm.branchPtr(Assembler::NotEqual, > + Address(object(), JSObject::offsetOfShape()), > + ImmGCPtr(obj->lastProperty()), &failures); > + > + // No need to emit if we can know statically. This comment doesn't make much sense. 'Guard that the incoming value is in the type set for the property, if a type barrier is required.' @@ +1975,5 @@ > + &barrierSuccess, &barrierFailure); > + } else { > + masm.guardTypeSet(valReg, propTypes, scratchReg, > + &barrierSuccess, &barrierFailure); > + } This |if| statement looks pretty strange, what keeps you from coalescing the two cases? Can you add a comment, or make some more changes to let the generic TypedOrValueRegister case work? @@ +2343,5 @@ > return false; > > + types::TypeObject *type = obj->getType(cx); > + if (cache.needsTypeBarrier() && !type->unknownProperties()) > + { Nit: { on the previous line. ::: js/src/jit/LIR-Common.h @@ +4133,5 @@ > // Patchable jump to stubs generated for a SetProperty cache, which stores a > // boxed value. > class LSetPropertyCacheV : public LInstructionHelper<0, 1 + BOX_PIECES, 1> > { > + bool needsTypeBarrier_; This flag can just be obtained from the MIR node instead. @@ +4160,5 @@ > // value of a known type. > class LSetPropertyCacheT : public LInstructionHelper<0, 2, 1> > { > MIRType valueType_; > + bool needsTypeBarrier_; Ditto.

Comment on attachment 793010 [details] [diff] [review] Part 2: Implement generic Proxy::set stub for SetPropertyIC Review of attachment 793010 [details] [diff] [review]: ----------------------------------------------------------------- Have you measured cases where these stubs get generated? Is there anyplace where this generic stub gets generated, but there are also a large number of passes which are more heavily optimizable that might get passed over because of the presence of this stub? Outside of that concern, patch logic looks good. ::: js/src/jit/IonCaches.cpp @@ +2067,5 @@ > return true; > } > > +static bool > +EmitCallProxySet(JSContext *cx, MacroAssembler &masm, IonCache::StubAttacher &attacher, The previous codegen helpers are all named |Generate...|. This one is named |Emit...|. Might be good to call this one |GenerateCallProxySet| as well. Especially since the Emit naming convention is more used by baseline code. @@ +2163,5 @@ > + > + Register scratch = regSet.takeGeneral(); > + masm.push(scratch); > + > + GenerateProxyClassGuards(masm, object(), scratch, &proxyFailures, &proxySuccess); Is there any way to expand these guards to be more than just "this is generically optimizable", and into "this is generically optimizable, and we're pretty sure there's no better optimized stubs we can create for this object."?

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ccfd8f31bf https://hg.mozilla.org/integration/mozilla-inbound/rev/7180f4ffb7f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/61ac97a7fc7c

https://hg.mozilla.org/mozilla-central/rev/c1ccfd8f31bf https://hg.mozilla.org/mozilla-central/rev/7180f4ffb7f1 https://hg.mozilla.org/mozilla-central/rev/61ac97a7fc7c