1063921 - Object.assign is slow

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Tom S [:evilpie]]

We should optimize Object.assign. One of the most prominent issues that I noticed with perf record (not always accurate) is that we create an arguments object for the function! I think that happens because the arguments-analysis stops at const MISSING = {}; with the reason: "No template object for NEWOBJECT".
After manually fixing this problem we abort again "Try-catch during arguments usage analysis"

Comment 1

[ziyunfei]

(In reply to Tom Schuster [:evilpie] from comment #0)
> We should optimize Object.assign. One of the most prominent issues that I
> noticed with perf record (not always accurate) is that we create an
> arguments object for the function!

May be we can use rest parameters to get arguments, like this:

>function ObjectStaticAssign(target, firstSource, ...sources) {
>    var to = ToObject(target);
>    sources.unshift(firstSource);
>    ...

or this:

>function ObjectStaticAssign(target, ...sources) {
>    ...
>}
>setFunctionLength(ObjectStaticAssign, 2);

Comment 2

[André Bargull [:anba]]

Attachment: bug1063921.patch

WIP - definitely needs some clean-up (comments, spec steps, better function names, ...).

Approx. four times faster compared to the current version (micro benchmark code below). Adding an intrinsic function to avoid allocating the property descriptor object gave the most speed-up. The logic for the IsEnumerableOwnProperty() intrinsic was copy-pasted from PropDesc::initFromPropertyDescriptor and PropDesc::makeObject, probably should have searched if there is an existing method for this functionality... 

Any recommendations on this approach, does it look sane? And is there a different benchmark I should have used to test the performance?


function t() {
  var source = {a:1, b:2, c:3};
  var target = {};
  var d = Date.now();
  for (var i = 0; i < 500000; ++i) Object.assign(target, source);
  return Date.now() - d;
}

Comment 3

[Tom S [:evilpie]]

Comment on attachment 8486764 [details] [diff] [review]
bug1063921.patch

Review of attachment 8486764 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! Removing that allocation was a good idea. I am going to give f- for now, because it's not really obvious.

::: js/src/builtin/Object.js
@@ +4,3 @@
>  
>  /* ES6 draft 2014-07-18 19.1.2.1. */
>  function ObjectStaticAssign(target, firstSource) {

If you want to continue working on this, please bring back all the step numbers.

@@ +35,3 @@
>          }
> +    } catch (e) {
> +        ObjectStaticAssign_Rest(nextIndex + 1, keysArray, to, from, e);

Okay I think I understand how this is supposed to work with pending exceptions.
So you wait for the first exception (if it happens), they you run _Rest and ignore all exceptions there and throw the first exception that occurred. Can we please go back to the old code, unless of course this makes a real performance difference. I assume usually the catch block isn't run, so I am not sure why it would.

::: js/src/vm/SelfHosting.cpp
@@ +122,5 @@
>                                JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS);
>  }
>  
> +static bool
> +intrinsic_IsEnumerableOwnProperty(JSContext *cx, unsigned argc, Value *vp)

Good idea!

@@ +134,5 @@
> +    RootedId id(cx);
> +    if (!ValueToId<CanGC>(cx, args.get(1), &id))
> +        return false;
> +    Rooted<PropertyDescriptor> desc(cx);
> +    if (!GetOwnPropertyDescriptor(cx, obj, id, &desc)) {

No { }

@@ +143,5 @@
> +        return true;
> +    }
> +    uint8_t attrs = uint8_t(desc.attributes());
> +    JS_ASSERT_IF(attrs & JSPROP_READONLY, !(attrs & (JSPROP_GETTER | JSPROP_SETTER)));
> +    bool hasEnumerable = !(desc.attributes() & JSPROP_IGNORE_ENUMERATE);

Okay somebody else should look at this.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8486764 [details] [diff] [review]
bug1063921.patch

Review of attachment 8486764 [details] [diff] [review]:
-----------------------------------------------------------------

It's annoying that Object.assign is specced to have a length of 2, making it much more annoying to use ...rest args.

Overall, I think splitting things up into a main loop and a body function is the right approach. I agree with evilpie that splitting things up even further to run as little code as possible under the try block doesn't make sense, however: try blocks affect performance very little nowadays (except for annoying cases like arguments analysis). What's more, the case where no exception is thrown is going to be the common case by a huge margin, so this optimization will only ever hit in, well, the exceptional case.

So, if you could simplify the implementation a bit again and add back the step comments, that'd be great.

Also, I'll file a follow-up bug about potentially inlining IsEnumerableOwnProperty in Ion. That should make Object.assign roughly as fast as it can be (for native non-proxy objects, at least).

::: js/src/vm/SelfHosting.cpp
@@ +143,5 @@
> +        return true;
> +    }
> +    uint8_t attrs = uint8_t(desc.attributes());
> +    JS_ASSERT_IF(attrs & JSPROP_READONLY, !(attrs & (JSPROP_GETTER | JSPROP_SETTER)));
> +    bool hasEnumerable = !(desc.attributes() & JSPROP_IGNORE_ENUMERATE);

Yeah, this is gnarly stuff. But apart from the fact that `desc.attributes()` can be replaced by `attrs`, this is as fine as it gets.

@@ +144,5 @@
> +    }
> +    uint8_t attrs = uint8_t(desc.attributes());
> +    JS_ASSERT_IF(attrs & JSPROP_READONLY, !(attrs & (JSPROP_GETTER | JSPROP_SETTER)));
> +    bool hasEnumerable = !(desc.attributes() & JSPROP_IGNORE_ENUMERATE);
> +    bool isEnumerable = ((attrs & JSPROP_ENUMERATE) != 0);

I don't think you need any coercion here at all, but if some compiler spews a warning, we usually do `!!(attrs & JSPROP_ENUMERATE)`.

@@ +838,4 @@
>      JS_FN("IsCallable",              intrinsic_IsCallable,              1,0),
>      JS_FN("IsConstructor",           intrinsic_IsConstructor,           1,0),
>      JS_FN("OwnPropertyKeys",         intrinsic_OwnPropertyKeys,         1,0),
> +    JS_FN("IsEnumerableOwnProperty", intrinsic_IsEnumerableOwnProperty, 2,0),

We're trying to establish a naming scheme where intrinsics start with "_", so please change the name accordingly.

Comment 5

[André Bargull [:anba]]

Attachment: Revised patch

I've requested review from both of you, but two separate reviews are not really necessary, whoever gets here first should probably remove the review flag for the other one.

I've applied all requested changes, the step numbers and variables names are now aligned to the rev27 draft. I've also simplified IsEnumerableOwnProperty a bit, so it's more obvious what's happening there. 

Apart from that, I wonder if we should just change Object.prototype.propertyIsEnumerable to work with proxy objects instead of adding the intrinsic function? :Waldo even already suggested to use Object.prototype.propertyIsEnumerable, but apparently there was some kind of issue with that approach (Bug 937855 comment 24). Possibly related to JSObject::lookupGeneric calling the "has" trap on the proxy object and traversing the complete prototype chain? IOW the current implementation of Object.prototype.propertyIsEnumerable is not spec compliant and if it gets changed to be spec compliant, the new intrinsic function is not needed.

Comment 6

[André Bargull [:anba]]

I've filed bug 1066834 to fix Object.prototype.propertyIsEnumerable.

Comment 7

[Tom S [:evilpie]]

Comment on attachment 8487937 [details] [diff] [review]
Revised patch

Review of attachment 8487937 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work. I think you are right about propertyIsEnumerable, feel free to implement that. The current comment in assign actually explains the problem with propertyIsEnumerable, right now it uses JSObject::lookupGeneric, which does the incorrect [[has]] call on proxies.

::: js/src/builtin/Object.js
@@ +15,3 @@
>      var i = 1;
>      do {
> +        // Step 5.a (Directly continue to the next iteration instead of

I think the step is enough, this optimization is quite obvious and this function doesn't even handle the list anymore.

@@ +31,5 @@
> +/*
> + * Object.assign steps 5.b-e, split from the main loop to allow arguments
> + * analysis to omit allocating an arguments object.
> + */
> +function objectAssignSingleSource(to, nextSource) {

Uppercase and change |nextSource| to just |source|. The comment can be C++ style with //.

::: js/src/vm/SelfHosting.cpp
@@ +829,5 @@
>      JS_FN("IsConstructor",           intrinsic_IsConstructor,           1,0),
>      JS_FN("OwnPropertyKeys",         intrinsic_OwnPropertyKeys,         1,0),
> +    JS_FN("_IsEnumerableOwnProperty",
> +          intrinsic_IsEnumerableOwnProperty,
> +          2, 0),

Please leave this on one line even when it doesn't line up with the others anymore.

Comment 8

[André Bargull [:anba]]

(In reply to Tom Schuster [:evilpie] from comment #7)
> Nice work. I think you are right about propertyIsEnumerable, feel free to
> implement that. The current comment in assign actually explains the problem
> with propertyIsEnumerable, right now it uses JSObject::lookupGeneric, which
> does the incorrect [[has]] call on proxies.
> 

Thanks for the review! I've set bug 1066834 as a blocker, after review has been granted for that bug, I'll update the patch for this bug to use Object.prototype.propertyIsEnumerable.

Comment 9

[André Bargull [:anba]]

Attachment: bug1063921-Object_assign.patch

Incorporated comments from Tom and updated patch to use Object.prototype.propertyIsEnumerable. Carrying over r=evilpies.

I've also removed "obj_getOwnPropertyDescriptor" from the list of builtins exposed to the self-hosting environment (it was only called in ObjectStaticAssign) and renamed the marker object from "MISSING" to "EMPTY" to match the rev28 placeholder [1].

I'm setting NI? for :till, because I still got a question about the new self-hosting initialization (bug 1067459). Is it necessary for the JS_FN "nargs" parameter for exported builtin functions to match the actual function length or does it need to include the implicit |this|? For example obj_hasOwnProperty is exported with nargs=2 even though Object.prototype.hasOwnProperty only takes a single parameter. Compared to that str_indexOf is exported with nargs=1 which does match the number of parameters for String.prototype.indexOf.


[1] https://bugs.ecmascript.org/show_bug.cgi?id=3231#c1

Comment 10

[Jeff Walden [:Waldo]]

(In reply to André Bargull from comment #9)
> Is it necessary for the JS_FN
> "nargs" parameter for exported builtin functions to match the actual
> function length or does it need to include the implicit |this|? For example
> obj_hasOwnProperty is exported with nargs=2 even though
> Object.prototype.hasOwnProperty only takes a single parameter. Compared to
> that str_indexOf is exported with nargs=1 which does match the number of
> parameters for String.prototype.indexOf.

Nothing should include the implicit |this|, and everything should be consistent with the initial value of the function's length property.  (My tree has hOP with narg=1, but maybe it's just out of date.)

Comment 11

[Till Schneidereit [:till]]

What Waldo said: no implicit `this`. (And my tree has hOP with nargs=1, too.)

What's a bit annoying is that default and rest args have to be included in nargs, so it doesn't alway match Function#length. Might/should change at some point, but for now it is what it is.

Comment 12

[André Bargull [:anba]]

(In reply to Till Schneidereit [:till] from comment #11)
> What Waldo said: no implicit `this`. (And my tree has hOP with nargs=1, too.)

nargs=2 is still used in central and inbound. So either you already have a local patch in place or we've talked past each other. ;-) I don't mean the definition for Object.prototype.hasOwnProperty in vm/Object.cpp, but the other in vm/SelfHosting.cpp [1, 2].

http://hg.mozilla.org/mozilla-central/file/78a4540b0a9c/js/src/vm/SelfHosting.cpp#l851
https://hg.mozilla.org/integration/mozilla-inbound/file/b3cf4699f538/js/src/vm/SelfHosting.cpp#l851

Comment 13

[Till Schneidereit [:till]]

> I don't mean the
> definition for Object.prototype.hasOwnProperty in vm/Object.cpp, but the
> other in vm/SelfHosting.cpp [1, 2].

Oh, I see. Yeah, that's totally bogus. r=me on including a fix in your next patch.

Comment 14

[Jeff Walden [:Waldo]]

If the length properties of any of the std_* methods actually affects the semantics of any self-hosted code, or of non-self-hosted code, I think we're doing something wrong, honestly.  Making them consistent is probably nice to avoid questions like this, but no self-hosted code should be inspecting self-hosted functions' lengths.  (In a saner language I don't think function lengths would even be exposed, but here we are.)

Comment 15

[Till Schneidereit [:till]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> If the length properties of any of the std_* methods actually affects the
> semantics of any self-hosted code, or of non-self-hosted code, I think we're
> doing something wrong, honestly.

We don't, this would just be for cleanup. Which is why I treated it so nonchalantly above.

Comment 16

[Carsten Book [:Tomcat]]

Hi, can you provide a try link for this changes ?

Comment 17

[André Bargull [:anba]]

I don't have try access, so someone else needs to push to try.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

This patch doesn't apply cleanly on inbound. If you rebase it and post it, I'll push it try for you.
I see you already have a few contributions in the tree. You can ask for level 1 commit access, which will provide you commit access to the try server. See [0] for details and feel free to open a bug for asking for commit access and CC till or evilpie (or anybody of the JS team) on it.

[0] https://www.mozilla.org/hacking/commit-access-policy/

Comment 19

[Till Schneidereit [:till]]

I'll gladly vouch for you, yes. Just CC me on the bug and I'll say so over there.

Comment 20

[André Bargull [:anba]]

Attachment: bug1063921_Object_assign_rebased.patch

Finally managed to rebase the patch - mercurial didn't let me clone m-inbound (first attempt failed during hg clone, second attempt lead to "integrity check failed" errors when performing hg refresh...).

Comment 21

[Tom S [:evilpie]]

The patch in bug 1103344 now uses propertyIsEnumerable and we don't have a try..catch anymore. Should be a lot faster now.

Comment 22

[Till Schneidereit [:till]]

André, this never landed. Is it still relevant, and if so, can it land?

Comment 23

[André Bargull [:anba]]

(In reply to Till Schneidereit [:till] from comment #22)
> André, this never landed. Is it still relevant, and if so, can it land?

No, it's no longer relevant, bug 1103344 fixed the previous performance issues. I think we can this one.

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to André Bargull from comment #23)
> (In reply to Till Schneidereit [:till] from comment #22)
> > André, this never landed. Is it still relevant, and if so, can it land?
> 
> No, it's no longer relevant, bug 1103344 fixed the previous performance
> issues. I think we can this one.

Assuming "I think we can *close* this one", please re-open if that guess is wrong.

Comment 25

[André Bargull [:anba]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #24)
> Assuming "I think we can *close* this one", please re-open if that guess is
> wrong.

Ups. Yes, you guessed correctly. :-)