Status

()

defect
P3
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: evilpie, Assigned: anba)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

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

5 years ago
(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);
Assignee

Comment 2

5 years ago
Posted patch bug1063921.patch (obsolete) — Splinter Review
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;
}
Attachment #8486764 - Flags: feedback?(evilpies)
Reporter

Comment 3

5 years ago
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.
Attachment #8486764 - Flags: feedback?(evilpies) → feedback-
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.
Assignee

Comment 5

5 years ago
Posted patch Revised patch (obsolete) — Splinter Review
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.
Assignee: nobody → andrebargull
Attachment #8486764 - Attachment is obsolete: true
Attachment #8487937 - Flags: review?(till)
Attachment #8487937 - Flags: review?(evilpies)
Assignee

Comment 6

5 years ago
I've filed bug 1066834 to fix Object.prototype.propertyIsEnumerable.
Reporter

Comment 7

5 years ago
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.
Attachment #8487937 - Flags: review?(evilpies) → review+
Assignee

Comment 8

5 years ago
(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.
Status: NEW → ASSIGNED
Depends on: 1066834
Assignee

Comment 9

5 years ago
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
Attachment #8487937 - Attachment is obsolete: true
Attachment #8487937 - Flags: review?(till)
Attachment #8504161 - Flags: review+
Flags: needinfo?(till)
(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.)
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.
Flags: needinfo?(till)
Assignee

Comment 12

5 years ago
(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
> 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.
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.)
(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.
Assignee

Updated

5 years ago
Keywords: checkin-needed
Hi, can you provide a try link for this changes ?
Keywords: checkin-needed
Assignee

Comment 17

5 years ago
I don't have try access, so someone else needs to push to try.
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/
Flags: needinfo?(andrebargull)
I'll gladly vouch for you, yes. Just CC me on the bug and I'll say so over there.
Assignee

Comment 20

5 years ago
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...).
Attachment #8504161 - Attachment is obsolete: true
Flags: needinfo?(andrebargull)
Attachment #8507059 - Flags: review+

Updated

5 years ago
Blocks: 1103344
Reporter

Comment 21

4 years ago
The patch in bug 1103344 now uses propertyIsEnumerable and we don't have a try..catch anymore. Should be a lot faster now.
André, this never landed. Is it still relevant, and if so, can it land?
Blocks: jsperf
Flags: needinfo?(andrebargull)
Priority: -- → P3
Assignee

Comment 23

3 years ago
(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.
Flags: needinfo?(andrebargull)
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Assignee

Comment 25

3 years ago
(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. :-)
You need to log in before you can comment on or make changes to this bug.