1394365 - Optimize property addition even more

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

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.)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Use templates for Qualified/Unqualified

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.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Add and use NativeObject::addEnumerableDataProperty

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.

Comment 3

[André Bargull [:anba]]

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.

Comment 4

[André Bargull [:anba]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 6

[Jan de Mooij [:jandem]]

(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.

Comment 7

[Jan de Mooij [:jandem]]

Filed bug 1394831.

Comment 8

[Jan de Mooij [:jandem]]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/43f2d93f5fa2
https://hg.mozilla.org/mozilla-central/rev/1c7831b29fd8