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

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
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.
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.
Blocks: 1351769
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
(Assignee)

Comment 2

a year ago
Created attachment 8873783 [details] [diff] [review]
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.
Attachment #8873783 - Flags: feedback?(jdemooij)
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.
Attachment #8873783 - Flags: feedback?(jdemooij) → feedback+
(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.
(Assignee)

Comment 5

a year ago
(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;
(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?
(Assignee)

Comment 7

a year ago
Created attachment 8874002 [details] [diff] [review]
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.
Assignee: nobody → andrebargull
Attachment #8873783 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8874002 - Flags: review?(jdemooij)
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.
Attachment #8874002 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 9

a year ago
(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.
(Assignee)

Comment 10

a year ago
Created attachment 8874743 [details] [diff] [review]
bug1368626.patch

Replaced NativeLookupOwnProperty<NoGC> with NativeLookupOwnPropertyNoResolve per comment #9. Carrying r+.
Attachment #8874002 - Attachment is obsolete: true
Attachment #8874743 - Flags: review+
Can we land this patch? It would be great to have it in 55 :)
(Assignee)

Comment 12

a year ago
(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
Keywords: checkin-needed

Comment 13

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad815515bd3
Avoid duplicate lookups when setting a new property. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ad815515bd3
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.