1368626 - NativeSetProperty should have a fast-path when adding a new property to avoid unnecessary property lookups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

When NativeSetProperty doesn't find an existing property, it calls SetPropertyByDefining from SetNonexistentProperty, and SetPropertyByDefining performs another property lookup, but most of the time this property lookup is a no-op because no existing property was found in NativeSetProperty [1].

SetPropertyByDefining then calls DefineProperty -> NativeDefineProperty, and NativeDefineProperty calls NativeLookupOwnProperty, resulting in another property lookup. 

We should optimize this case, so we don't perform these two additional lookups which are no-ops for the common case. 

[1] When NativeSetProperty's |obj| parameter == the |receiver| parameter.

Comment 1

[Jan de Mooij [:jandem]]

See also bug 1346217, where I optimized parts of the add-property path but there's definitely more work left.

Fixing this is pretty important. Property addition often shows up in profiles and our slow path for this is really slow.

Comment 2

[André Bargull [:anba]]

Attachment: bug1368626.patch

This avoids two no-op lookups (in SetPropertyByDefining and NativeDefineProperty) when setting a new property.

SetNonexistentProperty only takes the fast path for qualified sets, because I don't know if the HasProperty call NativeSetProperty can have side-effects which may make it invalid to use the fast path.

I also don't know if it's okay to take the fast path when |obj| has a getOpsGetOwnPropertyDescriptor hook, see the TODO in SetNonexistentProperty.

As a further optimization, I've moved the PurgeEnvironmentChain call in SetNonexistentProperty in the branch when a getOpsDefineProperty() is present, because for the other case, PurgeEnvironmentChain will also be called in AddOrChangeProperty.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8873783 [details] [diff] [review]
bug1368626.patch

Review of attachment 8873783 [details] [diff] [review]:
-----------------------------------------------------------------

This is excellent!

::: js/src/vm/NativeObject.cpp
@@ +1789,5 @@
> +
> +    // Optimized NativeDefineProperty() version for known absent properties.
> +
> +    // Dispense with custom behavior of exotic native objects first.
> +    if (obj->is<ArrayObject>()) {

It's interesting that all the exotic cases we handle here are indexes. I wonder if it's easy and worth it to add an "id is a PropertyName*" fast path as that's the common case?

@@ +1828,5 @@
> +
> +#ifdef DEBUG
> +    PropertyResult prop;
> +    NativeLookupOwnProperty<NoGC>(cx, obj, id, &prop);
> +    MOZ_ASSERT(!prop, "didn't expect to find an existing property");

Good assertion to have.

@@ +2518,5 @@
>  
> +    // Pure optimization for the common case. There's no point performing the
> +    // lookup in step 5.c again, as our caller just did it for us.
> +    if (qualified && receiver.isObject() && obj == &receiver.toObject()) {
> +        // TODO: Do we need to check for a custom obj->getOpsGetOwnPropertyDescriptor() here?

Maybe we can assert the behavior is consistent?

@@ +2548,5 @@
> +            if (!DefineNonexistentProperty(cx, obj, id, desc, result))
> +                return false;
> +        }
> +
> +        // If the receiver is native, there is one more legacy wrinkle: the class

Nit: the "If the receiver is native" part can be removed here.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> It's interesting that all the exotic cases we handle here are indexes. I
> wonder if it's easy and worth it to add an "id is a PropertyName*" fast path
> as that's the common case?

To answer myself, probably not as it's just a few branches right now and checking JSID_IS_INT/ATOM + checking for a PropertyName would also add some overhead.

Comment 5

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> @@ +1828,5 @@
> > +
> > +#ifdef DEBUG
> > +    PropertyResult prop;
> > +    NativeLookupOwnProperty<NoGC>(cx, obj, id, &prop);
> > +    MOZ_ASSERT(!prop, "didn't expect to find an existing property");
> 
> Good assertion to have.


Through the sandbox_resolve hook it's possible to trigger arbitrary side-effects, but I'm not sure the sandbox_resolve hook is a "proper" resolve hook implementation, because it allows side-effects:

var sandbox = evalcx("lazy");
Object.defineProperty(sandbox, "lazy", {
    get() {
        print("HI");
        Object.defineProperty(sandbox, "foo", { value: 0 });
    }
});

// Asserts "didn't expect to find an existing property" with the current patch
sandbox.foo = 1;

Comment 6

[Jan de Mooij [:jandem]]

(In reply to André Bargull from comment #5)
> Through the sandbox_resolve hook it's possible to trigger arbitrary
> side-effects, but I'm not sure the sandbox_resolve hook is a "proper"
> resolve hook implementation, because it allows side-effects:

Yeah it seems weird. Since it's shell-only it seems we can just define this "lazy" property as non-configurable and non-writable?

Comment 7

[André Bargull [:anba]]

Attachment: bug1368626.patch

Updated with the suggestions from comment #3. Also changed the "lazy" property of the shell-sandbox object to non-writable & non-configurable per comment #6 and added a test case for comment #5, which would fail if the "lazy" property could be redefined.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8874002 [details] [diff] [review]
bug1368626.patch

Review of attachment 8874002 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: js/src/vm/NativeObject.cpp
@@ +1827,5 @@
> +    }
> +
> +#ifdef DEBUG
> +    PropertyResult prop;
> +    NativeLookupOwnProperty<NoGC>(cx, obj, id, &prop);

Nit: should check for OOM here.

Comment 9

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> ::: js/src/vm/NativeObject.cpp
> @@ +1827,5 @@
> > +    }
> > +
> > +#ifdef DEBUG
> > +    PropertyResult prop;
> > +    NativeLookupOwnProperty<NoGC>(cx, obj, id, &prop);
> 
> Nit: should check for OOM here.

I think I want to change this call to use NativeLookupOwnPropertyNoResolve instead of NativeLookupOwnProperty<NoGC>, because the only reason I used NativeLookupOwnProperty<NoGC> was, that the NoGC-version doesn't call the resolve-hook.

Comment 10

[André Bargull [:anba]]

Attachment: bug1368626.patch

Replaced NativeLookupOwnProperty<NoGC> with NativeLookupOwnPropertyNoResolve per comment #9. Carrying r+.

Comment 11

[Jan de Mooij [:jandem]]

Can we land this patch? It would be great to have it in 55 :)

Comment 12

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #11)
> Can we land this patch? It would be great to have it in 55 :)

Yes, absolutely! I was just waiting one day to keep it separated from my other checkins from yesterday, so if there's a problem in one of them, the whole push wouldn't get backed-out. :-)


Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c2f4711c9d7c5e898216a8a1a893dded9170aa5

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad815515bd3
Avoid duplicate lookups when setting a new property. r=jandem

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ad815515bd3