1344469 - Optimize Object.hasOwnProperty

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Tom S [:evilpie]]

I have some WIP patches for this.

Comment 2

[Tom S [:evilpie]]

Attachment: Add JSOP_HASOWN

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)

Comment 3

[Tom S [:evilpie]]

Attachment: Baseline fallback support for HasOwn

Comment 4

[Tom S [:evilpie]]

Attachment: CacheIR for HasOwn with baseline support

Comment 5

[Tom S [:evilpie]]

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

Comment 6

[Tom S [:evilpie]]

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

Comment 7

[Tom S [:evilpie]]

Attachment: Megamorphic stub

Comment 8

[Tom S [:evilpie]]

Attachment: CacheIR HasOwn in Ion

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.

Comment 9

[Tom S [:evilpie]]

Attachment: Part 1 - Add JSOP_HASOWN

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.

Comment 10

[Tom S [:evilpie]]

Attachment: Part 2 - Baseline fallback support for HasOwn

Comment 11

[Tom S [:evilpie]]

Attachment: Part 3 - CacheIR for HasOwn with baseline support

Comment 12

[Tom S [:evilpie]]

Attachment: Part 4 - Megamorphic stub

Comment 13

[Tom S [:evilpie]]

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 14

[Tom S [:evilpie]]

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 15

[Jan de Mooij [:jandem]]

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 16

[Jan de Mooij [:jandem]]

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 17

[Jan de Mooij [:jandem]]

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 18

[Jan de Mooij [:jandem]]

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 19

[Jan de Mooij [:jandem]]

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.

Comment 20

[Jan de Mooij [:jandem]]

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.

Comment 21

[Tom S [:evilpie]]

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

Comment 22

[Tom S [:evilpie]]

Attachment: Part 6 - Port JSOP_IN notDefined optimization to JSOP_HASOWN

Comment 23

[Tom S [:evilpie]]

Attachment: micro benchmark

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.

Comment 24

[Tom S [:evilpie]]

Attachment: Part 5 - CacheIR HasOwn in Ion

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

Comment 25

[Tom S [:evilpie]]

Attachment: Test

Comment 26

[Tom S [:evilpie]]

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

Comment 27

[Tom S [:evilpie]]

Attachment: Implement more stuff for Speedometer

Comment 28

[Jan de Mooij [:jandem]]

evilpie++

Comment 29

[Jan de Mooij [:jandem]]

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 30

[Jan de Mooij [:jandem]]

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 31

[Tom S [:evilpie]]

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.

Comment 32

[Tom S [:evilpie]]

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca1453ae574
Part 6 - Port not defined property optimzation to HasOwn. r=jandem

Comment 35

[Iris Hsiao [:ihsiao]]

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

Comment 36

[Guilherme Lima]

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

Comment 37

[Jan de Mooij [:jandem]]

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.

Comment 38

[Tom S [:evilpie]]

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

Comment 39

[Jan de Mooij [:jandem]]

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

Comment 40

[:Harald Kirschner :digitarald]

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?

Comment 41

[Jan de Mooij [:jandem]]

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