Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize constructor.resolve lookup #40

Merged
merged 3 commits into from Apr 16, 2019

Conversation

Projects
None yet
5 participants
@gsathya
Copy link
Member

gsathya commented Apr 8, 2019

To optimize away the Get and Call of the resolve method on the constructor, I'd have to check the constructor to see if its resolve method has been modified or not. If it's not been modified, I can directly call (or even inline) the builtin %PromiseResolve%, saving the lookup and call overhead for faster performance.

Without this patch, I would have to check against the constructor for every iteration of the loop before going to the fast path.

With this patch, I can do a check against the constructor just once at the beginning.

The change in behavior with this patch is that if you modify the constructor's resolve property in the middle of iterating the iterable argument, then it is not observed. I don't think anyone should do this in any case as it's surprising side-effecting behavior.

@ljharb
Copy link
Member

ljharb left a comment

Should this change be pursued for https://tc39.github.io/ecma262/#sec-performpromiseall as well?

Show resolved Hide resolved spec.html Outdated
@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Apr 8, 2019

Should this change be pursued for https://tc39.github.io/ecma262/#sec-performpromiseall as well?

Promise.all, Promise.race, and Promse.any's proposal, too.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 8, 2019

It kind of seems like before this optimization lands here, there should be some consensus for making the change in the other promise methods?

(Especially since allSettled can’t be polyfilled with Promise.all if there’s an inconsistency)

@gsathya

This comment has been minimized.

Copy link
Member Author

gsathya commented Apr 8, 2019

It kind of seems like before this optimization lands here, there should be some consensus for making the change in the other promise methods?

That would mean blocking shipping this until we do the web compat dance for all the other methods, which seems unfortunate.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 9, 2019

I agree, but if we end up with a nice table of 4 Promise combinators, per the readme, but 3 of them have one observable behavior and 1 has another, that seems more unfortunate :-/

@mathiasbynens

This comment has been minimized.

Copy link
Member

mathiasbynens commented Apr 9, 2019

We have successfully made similar changes to the iterator protocol in the past without any web-compat trouble, which makes me hopeful that we'd be able to get away with these changes (to the two existing combinators) as well.

I agree, but if we end up with a nice table of 4 Promise combinators, per the readme, but 3 of them have one observable behavior and 1 has another, that seems more unfortunate :-/

Ack, but note that instead of 3-1 it would be 2-2 (since it seems the 2 new proposals would enable the optimization @gsathya suggested).

cc @anba who has a SpiderMonkey patch.

@gibson042

This comment has been minimized.

Copy link
Contributor

gibson042 commented Apr 9, 2019

I agree with @ljharb; at no point should this proposal specify behavior that differs from already-required common behavior in the other Promise combinators. If we can get Promise.all and Promise.race updated then I'm for this, but not otherwise.

@gsathya

This comment has been minimized.

Copy link
Member Author

gsathya commented Apr 10, 2019

Filed tc39/ecma262#1505 to track changes for Promise.all and Promise.race.

I understand the spec consistency argument for having all the promise combinators have the same behavior, but I think it's worthwhile to deviate in this very minor case if it means Promise.allSettled and Promise.any will have better performance.

We should try and change Promise.all and Promise.race -- I will implement this in V8 and get data to see if this possible. But, if we find it web incompatible to change Promise.all and Promise.race, then it's unfortunate that newer features have to pay the price and be slow as well.

@gibson042

This comment has been minimized.

Copy link
Contributor

gibson042 commented Apr 10, 2019

Thanks very much for opening the main spec issue!

As for these new functions, I disagree that deviation is worthwhile, and would further caution against overemphasizing the significance of performance upon Promise aggegration, which can surely tolerate some inefficiency.

@mathiasbynens

This comment has been minimized.

Copy link
Member

mathiasbynens commented Apr 16, 2019

Ok, to summarize the above:

  1. We'll ship the optimization for all existing promise combinators at the same time in V8/Chrome. We're hoping it'll be web-compatible, and will report back otherwise.
  2. I'm merging this PR now.

@mathiasbynens mathiasbynens merged commit d2a5c56 into tc39:master Apr 16, 2019

@gsathya gsathya deleted the gsathya:patch-3 branch Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.