Optimize property addition even more

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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

2 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

2 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)
Attachment #8901765 - Flags: review?(andrebargull) → review+
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+
(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! :-)

Comment 5

2 years ago
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

2 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

2 years ago
Filed bug 1394831.
(Assignee)

Comment 8

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/43f2d93f5fa2
https://hg.mozilla.org/mozilla-central/rev/1c7831b29fd8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.