A few weeks ago I noticed Speedometer spends a bunch of time under hasOwnProperty. I also noticed JSC has a HasOwnPropertyCache. It seems we could do much better than that if we do the following (inspired by bug 1344463): (1) Self-host hasOwnProperty. (2) Make it call an intrinsic that the bytecode emitter turns into an op like JSOP_IN (maybe JSOP_IN_OWN or JSOP_HAS_OWN_PROPERTY). (3) The JITs could share most of their JSOP_IN optimizations with this new op. That should make hasOwnProperty basically as fast as JSOP_IN or a JSOP_GETPROP.

I have some WIP patches for this.

Add the JSOP_HASOWN opcode that is going to be called from the self-hosted Object_hasOwnProperty function. I thought about doing the ToObject and ToPropertyKey in the self-hosted function, but that is going to involve two extra calls when not inlining (baseline/interp)

I am probably just going to wait for bug 1337773 before implementing Ion support independently.

Judging from emberjs/react we will also need megamorphic support for this op.

This is definitely the part with the most open questions for me. Should be always box the both inputs? I tried using the same policy as GetPropertyCache, but that caused issues in other places.

I replaced std_Object_hasOwnProperty everywhere with hasOwn, I am not sure if this actually useful. The parameter order is a bit weird, but matches 'in'. As well as order in Object#hasOwnProperty, where ToPropertyKey is called before ToObject.

I think everything, but the Ion patch can be reviewed. I also have to additional patches for porting the NotDefined analysis from Ion as well as definite property support, which is untested.

Comment on attachment 8856175 [details] [diff] [review] CacheIR HasOwn in Ion There are various things marked with XX where I wasn't sure what to do.

Comment on attachment 8856176 [details] [diff] [review] Part 1 - Add JSOP_HASOWN Review of attachment 8856176 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. Looks great, thanks for taking this on! Using hasOwn directly makes sense to me as it eliminates the s-h function call.

Comment on attachment 8856256 [details] [diff] [review] Part 2 - Baseline fallback support for HasOwn Review of attachment 8856256 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineIC.cpp @@ +1285,5 @@ > + MOZ_ASSERT(engine_ == Engine::Baseline); > + > + EmitRestoreTailCallReg(masm); > + > + // Sync for the decompiler. This reminds me, please double check you get the same error messages as before for things like ({}).hasOwnProperty.call(null) ::: js/src/jit/BaselineIC.h @@ +503,1 @@ > Nit: could remove 2 of the 3 blank lines here.

Comment on attachment 8856257 [details] [diff] [review] Part 3 - CacheIR for HasOwn with baseline support Review of attachment 8856257 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I was wondering if we could use InIRGenerator for this, but separating them may be less confusing.

Comment on attachment 8856258 [details] [diff] [review] Part 4 - Megamorphic stub Review of attachment 8856258 [details] [diff] [review]: ----------------------------------------------------------------- Please add a test for this if our test suite doesn't hit this code (that reminds me I should fix bug 1348790..) ::: js/src/jit/CacheIR.h @@ +184,5 @@ > \ > _(MegamorphicLoadSlotResult) \ > _(MegamorphicLoadSlotByValueResult) \ > _(MegamorphicStoreSlot) \ > + _(MegamorphicHasOwnResult) \ Maybe we could later use this for In too if we rename to MegamorphicHasProperty and add an IsOwn template parameter to the VM function, or something? We can worry about that later though. ::: js/src/jit/VMFunctions.cpp @@ +1757,5 @@ > + > + // Property not found. Watch out for Class hooks. > + if (MOZ_UNLIKELY(!nobj->is<PlainObject>())) { > + if (ClassMayResolveId(cx->names(), nobj->getClass(), id, nobj) || > + nobj->getClass()->getGetProperty()) // XXX other hooks I think lookup/has The fast path in obj_hasOwnProperty calls NativeLookupOwnProperty -> LookupOwnPropertyInline, and there we only check for resolve hooks as far as I can tell? So I think ClassMayResolveId will be sufficient here...

Comment on attachment 8856175 [details] [diff] [review] CacheIR HasOwn in Ion Review of attachment 8856175 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonIC.cpp @@ +48,5 @@ > return asGetNameIC()->temp(); > case CacheKind::In: > MOZ_CRASH("Baseline-specific for now"); > + case CacheKind::HasOwn: > + return asHasOwnIC()->maybeTemp(); // XXX can we use output() ? Using the output register should work. Adding a temp may be reasonable (I think the primitive CacheIR regalloc benefits more from the extra register than Ion's regalloc) but in this case we can use the output register as temp so it probably doesn't help much. ::: js/src/jit/MIR.h @@ +12547,5 @@ > }; > > +class MHasOwnCache > + : public MBinaryInstruction, > + public MixPolicy<BoxPolicy<0>, BoxPolicy<1>>::Data We should match the GetProp cache for this to avoid unnecessary guards and (un)boxing. If the lowering code is the same I don't know why it would fail. @@ +12557,5 @@ > + > + // The cache will invalidate if there are objects with e.g. lookup or > + // resolve hooks on the proto chain. setGuard ensures this check is not > + // eliminated. > + // setGuard(); We can remove this because the MIR instruction is effectful so it won't be (re)moved anyway.

I'm interested in performance numbers btw, also for the megamorphic case. hasOwnProperty is used a lot so it's super cool to have ICs for it.

(In reply to Jan de Mooij [:jandem] from comment #18) > ::: js/src/jit/VMFunctions.cpp > @@ +1757,5 @@ > > + > > + // Property not found. Watch out for Class hooks. > > + if (MOZ_UNLIKELY(!nobj->is<PlainObject>())) { > > + if (ClassMayResolveId(cx->names(), nobj->getClass(), id, nobj) || > > + nobj->getClass()->getGetProperty()) // XXX other hooks I think lookup/has > > The fast path in obj_hasOwnProperty calls NativeLookupOwnProperty -> > LookupOwnPropertyInline, and there we only check for resolve hooks as far as > I can tell? So I think ClassMayResolveId will be sufficient here... I think in theory we should look out for getOpsGetOwnPropertyDescriptor, but no well behaved native object should have that. I will look into adding an assert that none of the important ObjectOps are defined on native classes. IIRC environment objects are the exception still.

This benchmark improves from around 58ms to 32ms. While benchmarking on a simpler test case I noticed that we don't attach this stub for unboxed objects. Same for |in|. That should be easy to fix in a followup.

Turns out there was some mistake about assigning the CacheIR input and I was switching up obj and key. https://treeherder.mozilla.org/#/jobs?repo=try&revision=290044d8ce15160c36984e167db5e639ec82b886&selectedJob=91016619

Looks like speedometer actually runs into unboxed objects and array indexes, so probably need to fix that as well soon.

evilpie++

Comment on attachment 8857466 [details] [diff] [review] Part 6 - Port JSOP_IN notDefined optimization to JSOP_HASOWN Review of attachment 8857466 [details] [diff] [review]: ----------------------------------------------------------------- Cool.

Comment on attachment 8857614 [details] [diff] [review] Test Review of attachment 8857614 [details] [diff] [review]: ----------------------------------------------------------------- Thanks ::: js/src/jit-test/tests/cacheir/hasown.js @@ +24,5 @@ > + > +function key() { > + var sym = Symbol(), sym2 = Symbol(); > + var keys = [[sym, true], [sym2, false], > + ["a", true], ["b", false], Nit: indentation looks wrong

Comment on attachment 8857692 [details] [diff] [review] Implement more stuff for Speedometer I think this actually ok to review. The naming with unboxed and native is a bit confusing, but I can fix that later.

With this last patch should we remove convertUnboxedObjects from jsop_hasown in Ion? Not sure how that interacts with the not defined analysis though.

Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7deb7a5cb370 Part 1 - Add JSOP_HASOWN to Interpreter and BytecodeEmitter. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/50e8be7ee748 Part 2 - Baseline fallback support for HasOwn. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/07b10177f708 Part 3 - CacheIR for HasOwn with baseline support. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1a2ab034ae Part 4 - Megamorphic stub. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d036ef05e8 Part 5 - CacheIR HasOwn in Ion. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/56e3c896e6cc Test. r=jandem

Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca1453ae574 Part 6 - Port not defined property optimzation to HasOwn. r=jandem

https://hg.mozilla.org/mozilla-central/rev/7deb7a5cb370 https://hg.mozilla.org/mozilla-central/rev/50e8be7ee748 https://hg.mozilla.org/mozilla-central/rev/07b10177f708 https://hg.mozilla.org/mozilla-central/rev/5e1a2ab034ae https://hg.mozilla.org/mozilla-central/rev/a9d036ef05e8 https://hg.mozilla.org/mozilla-central/rev/56e3c896e6cc https://hg.mozilla.org/mozilla-central/rev/2ca1453ae574

These patches improved sunspider-tagcloud by 15% and six-speed-for-of-object-es5 by 45%, according to AWFY.

Comment on attachment 8857692 [details] [diff] [review] Implement more stuff for Speedometer Review of attachment 8857692 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CacheIR.cpp @@ +2034,5 @@ > PropertyResult prop; > if (!LookupOwnPropertyPure(cx_, obj, key, &prop)) > return false; > > + if (!prop.isFound()) File a follow up bug to make similar changes to JSOP_IN? @@ +2052,2 @@ > emitIdGuard(keyId, key); > + TestMatchingReceiver(writer, obj, nullptr, objId, &expandoId); Hm TestMatchingReceiver's shape argument is unused? We should remove it but followup is fine. Also TestMatchingReceiver guards on the expando object but we don't need that guard for non-expando properties (we only want a group guard). I wonder if it would be simpler to move the unboxed object case out of this method, but not sure about expando properties.

(In reply to Jan de Mooij [:jandem] from comment #37) > Comment on attachment 8857692 [details] [diff] [review] > Implement more stuff for Speedometer > > Review of attachment 8857692 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/CacheIR.cpp > @@ +2034,5 @@ > > PropertyResult prop; > > if (!LookupOwnPropertyPure(cx_, obj, key, &prop)) > > return false; > > > > + if (!prop.isFound()) > > File a follow up bug to make similar changes to JSOP_IN? > > @@ +2052,2 @@ > > emitIdGuard(keyId, key); > > + TestMatchingReceiver(writer, obj, nullptr, objId, &expandoId); > > Hm TestMatchingReceiver's shape argument is unused? We should remove it but > followup is fine. > > Also TestMatchingReceiver guards on the expando object but we don't need > that guard for non-expando properties (we only want a group guard). I wonder > if it would be simpler to move the unboxed object case out of this method, > but not sure about expando properties. Just to clarify, we don't need to guard on the expando shape or existance, when the property is contained by the unboxed object, because we can't delete to properties without a new group?

(In reply to Tom Schuster [:evilpie] from comment #38) > Just to clarify, we don't need to guard on the expando shape or existance, > when the property is contained by the unboxed object, because we can't > delete to properties without a new group? Correct.

Jan, just wanted to track it in this bug for posterity: As this started a a finding in speedometer, what kind if improvement did we see for that benchmark?

(In reply to :Harald Kirschner :digitarald from comment #40) > Jan, just wanted to track it in this bug for posterity: As this started a a > finding in speedometer, what kind if improvement did we see for that > benchmark? It's hard to say because Speedometer runs a bunch of different tests and then reports a single number like 60 points so you need pretty big wins to move that. Follow-up bug 1356315 will optimize the hasOwnProperty-inside-for-in pattern that shows up in a lot of JS code (like React and Ember) and that might be the bigger improvement.