Editorial: refactor PerformPromiseThen to allow no capability #1146

ljharb · 2018-03-21T11:36:34Z

Fixes #694.

I followed @domenic's steps in the linked issue here; I'm not sure if other changes are needed.

Specifically, which PerformPromiseThen calls should now be able to omit a capability?

domenic · 2018-03-21T15:06:53Z

Search for "throwawayCapability"

ljharb · 2018-03-21T16:57:06Z

@domenic thanks, updated!

domenic

A few small things missing. Sorry for the massive delay!

domenic · 2018-04-20T21:01:53Z

spec.html

+            1. If _resultCapability_ is present, then
+              1. Assert: _resultCapability_ is a PromiseCapability Record.
+            1. Else,
+              1. Set _resultCapability to *undefined*.


Missing _

I think maybe you should use "Let" here instead of "Set" since the variable doesn't technically exist yet? (Which might argue for moving the else clause into a single line, so the "block scoping" is less confusing.)

The variable exists because it's in the abstract operation signature, so I think "Set" is correct.

Thanks, I've added the underscore

Yeah, I guess that depends on how you interpret "is present", but your way makes sense too.

domenic · 2018-04-20T21:03:40Z

spec.html

@@ -38204,7 +38202,7 @@ <h1>AsyncGeneratorRequest Records</h1>
            </tr>
            <tr>
              <td>[[Capability]]</td>
-              <td>A PromiseCapability record</td>
+              <td>A PromiseCapability record, or *undefined*</td>


I think you meant to edit PromiseReaction, not AsyncGeneratorRequest.

domenic · 2018-04-20T21:07:28Z

spec.html

@@ -39095,11 +39094,14 @@ <h1>Promise.prototype.then ( _onFulfilled_, _onRejected_ )</h1>

        <!-- es6num="25.4.5.3.1" -->
        <emu-clause id="sec-performpromisethen" aoid="PerformPromiseThen">
-          <h1>PerformPromiseThen ( _promise_, _onFulfilled_, _onRejected_, _resultCapability_ )</h1>
+          <h1>PerformPromiseThen ( _promise_, _onFulfilled_, _onRejected_ [ , _resultCapability_ ] )</h1>
          <p>The abstract operation PerformPromiseThen performs the &ldquo;then&rdquo; operation on _promise_ using _onFulfilled_ and _onRejected_ as its settlement actions. The result is _resultCapability_'s promise.</p>


Should modify the intro text. Suggestion:

If resultCapability is passed, the result is stored by updating resultCapability's promise. (If it is not passed, then PerformPromiseThen is being called by a specification-internal operation where the result does not matter.)

ljharb · 2018-04-20T21:31:51Z

Thanks, updated!

ljharb · 2018-06-04T18:12:32Z

@anba @domenic @gsathya @msaboff any objections to merging this editorial change into ES2018?

domenic · 2018-06-04T18:43:19Z

I don't like spending resources on snapshots, but whatevs...

littledan · 2018-06-24T23:28:22Z

@MayaLekova We chatted about this change and it sounded like you might have had some reservations about it. Are any embedding environments depending on the throwaway Promise, based on embedder-specific hooks into the Promise lifecycle, even if it's unobservable without those hooks?

MayaLekova · 2018-06-25T15:12:14Z

For now it seems we won't fire the promise hooks on the throwawayPromise, don't know if this is the only change. @ak239 - please comment as well!

alexkozy · 2018-06-25T15:21:32Z

On DevTools side to implement async stacks we rely on kBefore and kAfter hook calls for throwaway promise:

each time when async function is suspended - we capture stack trace at this moment,
when async function is resumed next time - we use kBefore and kAfter hooks - between these hooks we use captured async stack as current one.

From implementation point of view I believe we can instrument microtask by itself with inspector-only hooks or we can stop using promise hooks at all by proper instrumentation async function suspended, async function resumed and async function finished. It will work for us but I am not sure about some external developers who relies on promise hooks.

littledan · 2018-06-25T15:28:07Z

@ak239 Could you explain more how DevTools currently takes advantage of the kBefore and kAfter events related to the throwawayPromise?

alexkozy · 2018-06-25T15:46:13Z

When function is suspended we get instrumentation call with async function promise as argument and record stack trace for this promise.
On await we record mapping between throwaway promise and async function promise and expect that when async function is running next time we will get kBefore hook with throwaway promise before that run and kAfter hook after. We use recorded mapping between throwaway and async function promise here to map these hooks to async function.

As external developer I think that I can use:

kInit when throwaway promise is created to capture stack,
kBefore and kAfter when async function is resumed to set captured stack as current one.

What we actually would like to have: instrumentation every time when async function suspended, every time when async function is resumed and once when async function finished.

alexkozy · 2018-06-26T00:13:08Z

On inspector side we can implement required for us instrumentation without using throwawayPromise: PoC. So from inspector point of view this PR looks good to me.

I am not sure how important it is to provide these hooks for developers.

bmeurer · 2018-07-10T12:24:31Z

@littledan @domenic @ljharb Any other roadblocks left to have this merged?

) Fixes tc39#694.

…o capability". This implements the editorial change in tc39/ecma262#1146 which removes the need to allocate the throwaway promise in await (the throwaway promise was not exposed to user JavaScript anyways, but this spec change allows us to rely on this behavior). Now we still need the throwaway promise for proper before and after events with both DevTools (which might change) and PromiseHooks. So if either DevTools or PromiseHooks is on, we call into %AwaitPromisesInit, which then allocates the throwaway promise and does all the other debugger/hooks related setup. This gives around 7% improvement on the doxbee-async-es2017-native and around 1-2% on the parallel-async-es2017-native benchmarks. Bug: v8:7253, v8:8285 Ref: tc39/ecma262#1146 Tbr: [email protected] Change-Id: I972ba0538ec8c00808e95b183603025c7e55a6d3 Cq-Include-Trybots: luci.chromium.try:linux_chromium_headless_rel;luci.chromium.try:linux_chromium_rel_ng;master.tryserver.blink:linux_trusty_blink_rel Reviewed-on: https://chromium-review.googlesource.com/c/1270798 Commit-Queue: Benedikt Meurer <[email protected]> Reviewed-by: Benedikt Meurer <[email protected]> Reviewed-by: Maya Lekova <[email protected]> Reviewed-by: Sathya Gunasekaran <[email protected]> Cr-Commit-Position: refs/heads/master@{#56506}

This CL introduces a new fast-path for `Promise.all(a)` for the case that elements in `a` are native promises, and the Promise.prototype and Promise function itself are intact. If so, we can skip the lookups of "resolve" on Promise and "then" on the result of invoking "resolve", which are both quite expensive, and we can instead directly call the PerformPromiseThen() operation on the element of `a`. In addition to that we don't need to create and chain a result promise, since this is only used when either async_hooks or DevTools are enabled. Otherwise it's a "throwaway promise" only used to satisfy the operation parameter signature (see tc39/ecma262#1146). This results in a significant performance improvement on `Promise.all()` heavy code. For example the parallel-promises-es2015-native test goes from around 84ms to roughly 68ms, which is almost a 20% improvement. Bug: v8:7253 Ref: tc39/ecma262#1146 Change-Id: Iab9c57edb26d13a467b0653fd8de6149c382efc6 Reviewed-on: https://chromium-review.googlesource.com/c/1293374 Reviewed-by: Sathya Gunasekaran <[email protected]> Reviewed-by: Maya Lekova <[email protected]> Commit-Queue: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{#56858}

ljharb added the editorial change label Mar 21, 2018

ljharb requested a review from domenic March 21, 2018 11:36

ljharb force-pushed the promise_undefined_capability branch from 578f37b to a1234e9 Compare March 21, 2018 16:57

ljharb force-pushed the promise_undefined_capability branch from a1234e9 to 9a77b53 Compare April 17, 2018 23:07

domenic suggested changes Apr 20, 2018

View reviewed changes

ljharb force-pushed the promise_undefined_capability branch from 9a77b53 to 71e8b85 Compare April 20, 2018 21:31

domenic approved these changes Apr 20, 2018

View reviewed changes

ljharb requested review from bterlson and bmeck April 20, 2018 21:37

ljharb assigned bterlson Apr 20, 2018

bterlson approved these changes Jun 4, 2018

View reviewed changes

bterlson assigned ljharb and unassigned bterlson Jun 4, 2018

bmeck approved these changes Jun 4, 2018

View reviewed changes

ljharb force-pushed the promise_undefined_capability branch from 71e8b85 to 414666d Compare June 4, 2018 18:12

ljharb force-pushed the promise_undefined_capability branch from 414666d to 689a396 Compare July 11, 2018 18:58

ljharb assigned bterlson and unassigned ljharb Jul 11, 2018

ljharb assigned ljharb and unassigned bterlson Aug 9, 2018

ljharb force-pushed the promise_undefined_capability branch from 689a396 to 5e93e1b Compare August 9, 2018 20:03

Editorial: refactor PerformPromiseThen to allow no capability (tc39#1146

a7139c5

) Fixes tc39#694.

ljharb force-pushed the promise_undefined_capability branch from 5e93e1b to a7139c5 Compare August 9, 2018 20:03

ljharb merged commit a7139c5 into tc39:master Aug 9, 2018

ljharb deleted the promise_undefined_capability branch August 9, 2018 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editorial: refactor PerformPromiseThen to allow no capability #1146

Editorial: refactor PerformPromiseThen to allow no capability #1146

ljharb commented Mar 21, 2018

domenic commented Mar 21, 2018 •

edited

Loading

ljharb commented Mar 21, 2018

domenic left a comment •

edited

Loading

domenic Apr 20, 2018

ljharb Apr 20, 2018

domenic Apr 20, 2018

domenic Apr 20, 2018

domenic Apr 20, 2018

ljharb commented Apr 20, 2018

ljharb commented Jun 4, 2018

domenic commented Jun 4, 2018

littledan commented Jun 24, 2018

MayaLekova commented Jun 25, 2018

alexkozy commented Jun 25, 2018

littledan commented Jun 25, 2018

alexkozy commented Jun 25, 2018

alexkozy commented Jun 26, 2018 •

edited

Loading

bmeurer commented Jul 10, 2018

Editorial: refactor PerformPromiseThen to allow no capability #1146

Editorial: refactor PerformPromiseThen to allow no capability #1146

Conversation

ljharb commented Mar 21, 2018

domenic commented Mar 21, 2018 • edited Loading

ljharb commented Mar 21, 2018

domenic left a comment • edited Loading

Choose a reason for hiding this comment

domenic Apr 20, 2018

Choose a reason for hiding this comment

ljharb Apr 20, 2018

Choose a reason for hiding this comment

domenic Apr 20, 2018

Choose a reason for hiding this comment

domenic Apr 20, 2018

Choose a reason for hiding this comment

domenic Apr 20, 2018

Choose a reason for hiding this comment

ljharb commented Apr 20, 2018

ljharb commented Jun 4, 2018

domenic commented Jun 4, 2018

littledan commented Jun 24, 2018

MayaLekova commented Jun 25, 2018

alexkozy commented Jun 25, 2018

littledan commented Jun 25, 2018

alexkozy commented Jun 25, 2018

alexkozy commented Jun 26, 2018 • edited Loading

bmeurer commented Jul 10, 2018

domenic commented Mar 21, 2018 •

edited

Loading

domenic left a comment •

edited

Loading

alexkozy commented Jun 26, 2018 •

edited

Loading