Closed
Bug 1394365
Opened 7 years ago
Closed 7 years ago
Optimize property addition even more
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
13.02 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
10.05 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
Adding a new property via NativeSetProperty is very common (o[x] = y is hard to optimize with ICs and Object.assign also calls this). We've done a lot of work to optimize this over the past months, but there's still some unnecessary slowness, for instance: (1) We convert between Value <-> PropertyDescriptor. (2) We get stuck in very generic code so we have lots of branches that are totally irrelevant for the common case. Things like: - Are we doing a qualified set? (Yes.) - Do we have a getter/setter to root? (No.) - Do we need to allocate a slot? (Yes.) - Is a dictionary mode conversion allowed? (Yes.) - Did we just add a writable property? (Yes.) etc etc. Each of these checks is pretty fast but it adds up. I have a few patches to optimize this. The micro-benchmark in bug 1372182 comment 0 improves from ~690 ms to ~588 ms, so about a ~15% improvement, pretty nice. (This is a bit simpler now with bug 1389510 fixed, because it got rid of the "use class getter/setter instead of a plain data prop" case.)
Assignee | ||
Comment 1•7 years ago
|
||
NativeSetProperty and SetNonexistentProperty have a QualifiedBool argument, but it's faster to make this a template argument. I also removed some unnecessary |proto| roots in the loops in NativeGetPropertyInline and NativeSetProperty. We only have to root in the uncommon cases.
Attachment #8901765 -
Flags: review?(andrebargull)
Assignee | ||
Comment 2•7 years ago
|
||
This is the second half. It changes DefineNonexistentProperty to take a HandleValue instead of Handle<PropertyDescriptor>. Then it avoids the AddOrChangeProperty call for non-integer jsids. Instead we call NativeObject::addEnumerableDataProperty. NativeObject::addEnumerableDataProperty duplicates some code in addPropertyInternal and getChildProperty. I think that's okay considering how hot this code is. The result is pretty nice because there are only a few cases to consider now so the code is much tighter. Longer term I think we should split addProperty(Internal) and maybe putProperty into separate data-prop vs accessor-prop methods.
Attachment #8901771 -
Flags: review?(andrebargull)
Updated•7 years ago
|
Attachment #8901765 -
Flags: review?(andrebargull) → review+
Comment 3•7 years ago
|
||
Comment on attachment 8901771 [details] [diff] [review] Part 2 - Add and use NativeObject::addEnumerableDataProperty Review of attachment 8901771 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Jan de Mooij [:jandem] from comment #2) > NativeObject::addEnumerableDataProperty duplicates some code in > addPropertyInternal and getChildProperty. I think that's okay considering > how hot this code is. The result is pretty nice because there are only a few > cases to consider now so the code is much tighter. > > Longer term I think we should split addProperty(Internal) and maybe > putProperty into separate data-prop vs accessor-prop methods. Yes, we should definitely consider this. Can you file a follow-up bug for this? (We could add some MOZ_ALWAYS_INLINE methods to reduce the code duplication, but it probably makes more sense to attempt splitting addProperty(Internal) before adding more tiny inline-helper methods.) ::: js/src/vm/NativeObject.cpp @@ +1948,5 @@ > // Step 2. > if (!obj->nonProxyIsExtensible()) > return result.fail(JSMSG_CANT_DEFINE_PROP_OBJECT_NOT_EXTENSIBLE); > > + if (MOZ_UNLIKELY(JSID_IS_INT(id))) { Will this make int-jsids slower? @@ +1957,5 @@ > + desc.setDataDescriptor(v, JSPROP_ENUMERATE); > + > + if (!AddOrChangeProperty<IsAddOrChange::Add>(cx, obj, id, desc)) > + return false; > + } else { I think I'd prefer to have this block moved into a separate method (placed below AddOrChangeProperty), so it's easier to see that it's a modified version of AddOrChangeProperty.
Attachment #8901771 -
Flags: review?(andrebargull) → review+
Comment 4•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #0) > I have a few patches to optimize this. The micro-benchmark in bug 1372182 > comment 0 improves from ~690 ms to ~588 ms, so about a ~15% improvement, > pretty nice. > This is great! :-)
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/43f2d93f5fa2 part 1 - Make NativeSetProperty's QualifiedBool argument a template parameter. r=anba https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7831b29fd8 part 2 - Add and use NativeObject::addEnumerableDataProperty. r=anba
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to André Bargull from comment #3) > > + if (MOZ_UNLIKELY(JSID_IS_INT(id))) { > > Will this make int-jsids slower? I removed the MOZ_UNLIKELY, you're right it's probably not adding much here. > I think I'd prefer to have this block moved into a separate method (placed > below AddOrChangeProperty), so it's easier to see that it's a modified > version of AddOrChangeProperty. Heh nice, that's what I had at first :) Moved it into an AddDataProperty function. I'll file a follow-up bug for the accessor/data prop split.
Assignee | ||
Comment 7•7 years ago
|
||
Filed bug 1394831.
Assignee | ||
Comment 8•7 years ago
|
||
Just one data point so far, but AWFY is saying this improved six-speed-object-assign-es6 by 15.7%. We're much closer to JSC now.
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/43f2d93f5fa2 https://hg.mozilla.org/mozilla-central/rev/1c7831b29fd8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•