+++ This bug was initially created as a clone of Bug #1209118 +++ Just like bug 1209118, but for SETPROP/SETELEM. This is even nicer than that bug, because the SETELEM IC currently only handles indexed properties, while SETPROP has all the native/setter/proxy goodness, so we'll hopefully get that for SETELEM as well.

Pretty straight-forward.

I think we no longer need this hack. Let's see what the benchmarks do.

Moves the dense and typed array stubs to the SetPropertyIC.

10 files changed, 0 insertions(+), 314 deletions(-)

Using SETELEM to set/add string/symbol properties is way faster now: before: 6084 ms after: 276 ms function f() { var sym = Symbol(); var s = "foo"; for (var i=0; i<10000000; i++) { var o = {}; o[sym] = i; o[s] = 3; } } var t = new Date; f(); print(new Date - t);

13 files changed, 399 insertions(+), 520 deletions(-)

8 day review ping ?

Comment on attachment 8674455 [details] [diff] [review] Part 1 - Add extra operand Review of attachment 8674455 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonCaches.cpp @@ +3561,5 @@ > cache.getScriptedLocation(&script, &pc); > MOZ_ASSERT(!script->hasNonSyntacticScope()); > InitGlobalLexicalOperation(cx, &cx->global()->lexicalScope(), script, pc, value); > } else { > + RootedPropertyName name(cx, idval.toString()->asAtom().asPropertyName()); Ah, OK. This will do for now, but we will have to make this branching logic more complicated. I assume this happens in the coming patches. Maybe a MOZ_ASSERT there that this will work is customary, though it's certainly entailed in the accessors.

Comment on attachment 8674458 [details] [diff] [review] Part 2 - Use MSetPropertyCache for SETELEM Review of attachment 8674458 [details] [diff] [review]: ----------------------------------------------------------------- There's a ton of guardHoles plumbing here, but it seems like it's never called in cache code? Should it have been? ::: js/src/jit/CodeGenerator.cpp @@ +8478,5 @@ > > void > CodeGenerator::addSetPropertyCache(LInstruction* ins, LiveRegisterSet liveRegs, Register objReg, > Register tempReg, ConstantOrRegister id, ConstantOrRegister value, > + bool strict, bool needsTypeBarrier, bool guardHoles, We could consider to have a uint8_t for all these bools, but I don't care much. ::: js/src/jit/IonBuilder.cpp @@ +9835,5 @@ > current->add(ins); > current->push(value); > > + if (guardHoles) > + ins->setGuardHoles(); Why not put this in the constructor? ::: js/src/jit/IonCaches.cpp @@ +1440,5 @@ > + if (this->id().constant()) > + return; > + > + EmitIdGuard(masm, id, this->id().reg(), object(), temp(), fail); > +} Nice. This is a pleasant refactor in line with the current naming schemes. Thanks. @@ +3270,5 @@ > static void > GenerateSetUnboxed(JSContext* cx, MacroAssembler& masm, IonCache::StubAttacher& attacher, > JSObject* obj, jsid id, uint32_t unboxedOffset, JSValueType unboxedType, > + Register object, Register tempReg, ConstantOrRegister value, bool checkTypeset, > + Label* failure) can we keep this standard with everything else? I think |failures| is more customary when it's an argument

Comment on attachment 8674461 [details] [diff] [review] Part 3 - Remove check Review of attachment 8674461 [details] [diff] [review]: ----------------------------------------------------------------- OK.

Comment on attachment 8674463 [details] [diff] [review] Part 4 - Move stubs from SetElementIC to SetPropertyIC Review of attachment 8674463 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonCaches.cpp @@ +4401,4 @@ > MacroAssembler masm(cx, ion, outerScript, profilerLeavePc_); > StubAttacher attacher(*this); > if (!GenerateSetDenseElement(cx, masm, attacher, obj, idval, > + guardHoles(), object(), id().reg(), aha! ::: js/src/jit/shared/LIR-shared.h @@ +5933,5 @@ > setOperand(0, object); > setTemp(0, temp); > + setTemp(1, tempToUnboxIndex); > + setTemp(2, tempDouble); > + setTemp(3, tempFloat32); nit: trailing whitespace on tempFloat32 line.

Comment on attachment 8674464 [details] [diff] [review] Part 5 - Remove SetElement IC Review of attachment 8674464 [details] [diff] [review]: ----------------------------------------------------------------- These are so cathartic.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bfb54d60c85 https://hg.mozilla.org/integration/mozilla-inbound/rev/23218bc0843c https://hg.mozilla.org/integration/mozilla-inbound/rev/6176f9dc8f60 https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b9dba91af5 https://hg.mozilla.org/integration/mozilla-inbound/rev/55a2293db821

According to AWFY, this was a 31-33% win on Dromaeo's jslib-attr-prototype test :) https://arewefastyet.com/#machine=17&view=single&suite=dromaeo&subtest=jslib-attr-prototype

https://hg.mozilla.org/mozilla-central/rev/3bfb54d60c85 https://hg.mozilla.org/mozilla-central/rev/23218bc0843c https://hg.mozilla.org/mozilla-central/rev/6176f9dc8f60 https://hg.mozilla.org/mozilla-central/rev/b4b9dba91af5 https://hg.mozilla.org/mozilla-central/rev/55a2293db821