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.

Sign up for GitHub

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

Jump to bottom

Normative: Lookup constructor.resolve only once in PerformPromise{All, Race} #1506

Merged
merged 1 commit into from
Jun 15, 2019
Merged

Normative: Lookup constructor.resolve only once in PerformPromise{All, Race} #1506

merged 1 commit into from
Jun 15, 2019

Conversation

gsathya
Copy link
Member

@gsathya gsathya commented Apr 11, 2019

Fixes #1505

@gsathya gsathya mentioned this pull request Apr 11, 2019
Closed
leobalter
leobalter approved these changes Apr 11, 2019
View reviewed changes
Copy link
Member

@leobalter leobalter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed spec text seems correct.

@mathiasbynens mathiasbynens added needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Apr 11, 2019
@zenparsing zenparsing added the needs consensus This needs committee consensus before it can be eligible to be merged. label Apr 11, 2019
zenparsing
zenparsing reviewed Apr 11, 2019
View reviewed changes
spec.html Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Apr 11, 2019

Early throwing also matches Map etc, which check before iteration. (They do Get + IsCallable rather than GetMethod.)

@gsathya
Copy link
Member Author

gsathya commented Apr 11, 2019

Early throwing also matches Map etc, which check before iteration. (They do Get + IsCallable rather than GetMethod.)

SGTM, I hear folks like consistency. PR updated.

@ljharb ljharb self-assigned this Apr 11, 2019
leobalter added a commit to leobalter/test262 that referenced this pull request Apr 17, 2019
@leobalter
Add tests for Promise.all|race resolve lookup
f1f495b 
Ref tc39/ecma262#1506
@leobalter leobalter mentioned this pull request Apr 17, 2019
Merged
leobalter added a commit to leobalter/test262 that referenced this pull request Apr 24, 2019
@leobalter
Add tests for Promise.all|race resolve lookup
18faa66 
Ref tc39/ecma262#1506
leobalter added a commit to leobalter/test262 that referenced this pull request Apr 24, 2019
@leobalter
Add tests for Promise.all|race resolve lookup
b646cf6 
Ref tc39/ecma262#1506
oleavr pushed a commit to frida/v8 that referenced this pull request Apr 26, 2019
@gsathya
[promise] Lookup the resolve property only once
9c0c876 
In the PerformPromise{All, Race, AllSettled} operations, the resolve
property of the constructor is looked up only once.

In the implementation, for the fast path, where the constructor's
resolve property is untainted, the resolve function is set to undefined.
Since undefined can't be a valid value for the resolve function,
we can switch on it (in CallResolve) to directly call the  PromiseResolve
builtin. If the resolve property is tainted, we do an observable property
lookup, save this value, and call this property later (in CallResolve).

I ran this CL against the test262 tests locally and they all pass:
tc39/test262#2131

Spec:
- tc39/ecma262#1506
- tc39/proposal-promise-allSettled#40

Bug: v8:9152
Change-Id: Icb36a90b5a244a67a729611c7b3315d2c29de6e9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1574705
Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60957}
zloirock added a commit to zloirock/core-js that referenced this pull request May 2, 2019
@zloirock
lookup PromiseConstructor.resolve only once in Promise combinators
d6f6d0b 
per tc39/ecma262#1506
@gsathya gsathya added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Jun 6, 2019
@gsathya
Copy link
Member Author

gsathya commented Jun 6, 2019

This has reached consensus at the TC39 meeting today.

@ljharb ljharb added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Jun 6, 2019
@zenparsing zenparsing mentioned this pull request Jun 6, 2019
Merged
@ljharb ljharb force-pushed the optimize-promise-resolve-lookup branch from 295858d to 25745fe Compare June 15, 2019 22:44
@ljharb ljharb merged commit 25745fe into tc39:master Jun 15, 2019
@ljharb ljharb changed the title Lookup constructor.resolve only once in PerformPromise{All, Race} Normative: Lookup constructor.resolve only once in PerformPromise{All, Race} Jun 15, 2019
caiolima pushed a commit to caiolima/webkit that referenced this pull request Jun 20, 2019
Optimize resolve method lookup in Promise static methods
7bff1ab 
https://bugs.webkit.org/show_bug.cgi?id=198864

Patch by Alexey Shvayka <shvaikalesh@gmail.com> on 2019-06-19
Reviewed by Yusuke Suzuki.

JSTests:

* test262/expectations.yaml: Mark 18 test cases as passing.

Source/JavaScriptCore:

Lookup `resolve` method only once in Promise.{all,allSettled,race}.
(tc39/ecma262#1506)

Already implemented in V8.

* builtins/PromiseConstructor.js:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@246620 268f45cc-cd09-0410-ab3c-d52691b4dbfc
ryanhaddad pushed a commit to WebKit/WebKit that referenced this pull request Dec 22, 2020
@shvaikalesh @webkit-commit-queue
Optimize resolve method lookup in Promise static methods
83b3a1e 
https://bugs.webkit.org/show_bug.cgi?id=198864

Patch by Alexey Shvayka <shvaikalesh@gmail.com> on 2019-06-19
Reviewed by Yusuke Suzuki.

JSTests:

* test262/expectations.yaml: Mark 18 test cases as passing.

Source/JavaScriptCore:

Lookup `resolve` method only once in Promise.{all,allSettled,race}.
(tc39/ecma262#1506)

Already implemented in V8.

* builtins/PromiseConstructor.js:

Canonical link: https://commits.webkit.org/213010@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@246620 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibson042 gibson042 gibson042 left review comments

@ljharb ljharb ljharb approved these changes

@littledan littledan littledan approved these changes

@leobalter leobalter leobalter approved these changes

@zenparsing zenparsing zenparsing approved these changes

Assignees

@ljharb ljharb

Labels
has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lookup resolve property only once in PerformPromiseAll and PerformPromiseRace
8 participants
@gsathya @bakkot @ljharb @littledan @leobalter @gibson042 @zenparsing @mathiasbynens