Closed
Bug 1368626
Opened 7 years ago
Closed 7 years ago
NativeSetProperty should have a fast-path when adding a new property to avoid unnecessary property lookups
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
References
(Blocks 3 open bugs)
Details
Attachments
(1 file, 2 obsolete files)
12.90 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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]
Updated•7 years ago
|
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p1]
Assignee | ||
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
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+
Comment 4•7 years ago
|
||
(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•7 years 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;
Comment 6•7 years ago
|
||
(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•7 years ago
|
||
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 8•7 years ago
|
||
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•7 years 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•7 years ago
|
||
Replaced NativeLookupOwnProperty<NoGC> with NativeLookupOwnPropertyNoResolve per comment #9. Carrying r+.
Attachment #8874002 -
Attachment is obsolete: true
Attachment #8874743 -
Flags: review+
Comment 11•7 years ago
|
||
Can we land this patch? It would be great to have it in 55 :)
Assignee | ||
Comment 12•7 years 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•7 years 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
Comment 14•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7ad815515bd3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•