Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2317383002:
Async/await Promise dependency graph (Closed)
|
Created:
4 years, 3 months ago by Dan Ehrenberg Modified:
4 years, 3 months ago CC:
caitp, gsathya, neis, v8-reviews_googlegroups.com, Yang Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionAsync/await Promise dependency graph
This patch knits together Promises returned by async/await such that when
one async function awaits the result of another one, catch prediction works
across the boundaries, whether the exception comes synchronously or
asynchronously. Edges are added in three places:
- When a locally uncaught await happens, if the value passed into await
is a Promise, from the awaited value to the Promise under construction
in the broader async function
- From a "throwaway" Promise, which may be found on the Promise debug
stack, to the Promise under construction in the async function that
surrounds it
- When a Promise is resolved with another Promise (e.g., when returning a
Promise from an async function)
BUG=v8:5167
Committed: https://crrev.com/7265fdde7c76b9f875b40b0b139515936d491d64
Cr-Commit-Position: refs/heads/master@{#39522}
Patch Set 1 #Patch Set 2 : CLeanup #Patch Set 3 : CLeanup #Patch Set 4 : Only if debug is active #Patch Set 5 : Only if debug is active #
Total comments: 8
Patch Set 6 : Rebase #Patch Set 7 : rebase #Patch Set 8 : rebase #Patch Set 9 : rebase #Patch Set 10 : Better comments #Patch Set 11 : rebase #Patch Set 12 : Problematic test case #Patch Set 13 : Maintain visited set to break dependency cycles #
Total comments: 17
Patch Set 14 : Cleanups from review #Patch Set 15 : Fix parser #
Total comments: 2
Patch Set 16 : Rebase #Patch Set 17 : Reorder #Patch Set 18 : Store .promise in FunctionState #Patch Set 19 : Revert function state change, and mark throwaway promise as handled #Patch Set 20 : Fix comment #Patch Set 21 : Slight cleanup #Patch Set 22 : Rebase #Patch Set 23 : Rebase #Patch Set 24 : format #
Total comments: 11
Patch Set 25 : Remove cycle detection #Patch Set 26 : nopos #Patch Set 27 : Attempt to clean up dependency graph #Patch Set 28 : Attempt to clean up dependency graph #Patch Set 29 : Remove irrelevant whitespace change #
Total comments: 2
Patch Set 30 : let to var #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 102 (74 generated)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23562)
Description was changed from ========== Fix all the bugs Trying to build dependency graph BUG= ========== to ========== Async/await Promise dependency graph This patch knits together Promises returned by async/await such that when one async function awaits the result of another one, catch prediction works across the boundaries, whether the exception comes synchronously or asynchronously. Edges are added in three places: - When a locally uncaught await happens, if the value passed into await is a Promise, from the awaited value to the Promise under construction in the broader async function - From a "throwaway" Promise, which may be found on the Promise debug stack, to the Promise under construction in the async function that surrounds it - When a Promise is resolved with another Promise (e.g., when returning a Promise from an async function) BUG=v8:5167 ==========
littledan@chromium.org changed reviewers: + adamk@chromium.org, jgruber@chromium.org
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
littledan@chromium.org changed reviewers: + kozyatinskiy@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2317383002/diff/80001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/80001/src/js/promise.js#newco... src/js/promise.js:290: if (!IS_UNDEFINED(resolution) && DEBUG_IS_ACTIVE && IsPromise(resolution)) { nit: format, s/DEBUG_IS_ACTIVE/instrumenting, and can you make instrumenting the first conditional on this?
https://codereview.chromium.org/2317383002/diff/80001/src/js/harmony-async-aw... File src/js/harmony-async-await.js (left): https://codereview.chromium.org/2317383002/diff/80001/src/js/harmony-async-aw... src/js/harmony-async-await.js:73: return PerformPromiseThen(promise, onFulfilled, onRejected, AsyncFunctionAwait used to return something, but now it returns undefined. Was that intentional? https://codereview.chromium.org/2317383002/diff/80001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2317383002/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:4873: VariableProxy* temp_proxy = factory()->NewVariableProxy(temp_var); Not new to your code, but It seems very wrong that this proxy is referenced in three places. https://codereview.chromium.org/2317383002/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:4895: Token::ASSIGN, temp_proxy, async_function_await, nopos); As noted in the JS file, you're just assigning undefined here.
https://codereview.chromium.org/2317383002/diff/80001/src/js/harmony-async-aw... File src/js/harmony-async-await.js (left): https://codereview.chromium.org/2317383002/diff/80001/src/js/harmony-async-aw... src/js/harmony-async-await.js:73: return PerformPromiseThen(promise, onFulfilled, onRejected, On 2016/09/08 at 22:07:31, adamk wrote: > AsyncFunctionAwait used to return something, but now it returns undefined. Was that intentional? No one calls this; they call AsyncFunctionAwaitCaught/AsyncFunctionAwaitUncaught, which return the outer promise that's passed into them. We used to return the intermediate Promise, which would end up resolving to the same thing, but the new form is cleaner and more closely resembles the spec. Whatever the return value bubbles up to the user because the first "await"'s return value is the actual Promise that the async function returns. https://codereview.chromium.org/2317383002/diff/80001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2317383002/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:4873: VariableProxy* temp_proxy = factory()->NewVariableProxy(temp_var); On 2016/09/08 22:07:31, adamk wrote: > Not new to your code, but It seems very wrong that this proxy is referenced in > three places. I believe we can eliminate the second and third usage sites; will do in a follow-on patch. https://codereview.chromium.org/2317383002/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:4895: Token::ASSIGN, temp_proxy, async_function_await, nopos); On 2016/09/08 22:07:31, adamk wrote: > As noted in the JS file, you're just assigning undefined here. I believe it's assigning .promise, but there's not much point to that; I can just say that the value of the do-expression is .promise. Will clean up in the same follow-on patch.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23728)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with comments and modulo the layout test failures. https://codereview.chromium.org/2317383002/diff/220001/src/js/collection.js File src/js/collection.js (right): https://codereview.chromium.org/2317383002/diff/220001/src/js/collection.js#n... src/js/collection.js:488: to.SetAdd = SetAdd; This may not be necessary (see comments at import sites). https://codereview.chromium.org/2317383002/diff/220001/src/js/harmony-async-a... File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2317383002/diff/220001/src/js/harmony-async-a... src/js/harmony-async-await.js:83: throwawayCapability); Nit: needs formatting. https://codereview.chromium.org/2317383002/diff/220001/src/js/harmony-async-a... src/js/harmony-async-await.js:85: if (debug_is_active) { Should this also check for !IS_UNDEFINED(outerPromise)? Maybe this block could be merged with the other promiseAwaitHandlerSymbol block above if it's moved until after NewPromiseCapability and before PerformPromiseThen. https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js#newc... src/js/promise.js:33: var SetAdd; Could you do SetAdd = GlobalSet.prototype.add here instead of the export/import? https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js#newc... src/js/promise.js:585: // with another Promise, or an intermediate, hidden, throaway Promise throwaway https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js#newc... src/js/promise.js:590: if (outerPromise && Would it make sense to make this stricter using IsPromise(outerPromise)? https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js#newc... src/js/promise.js:619: // main contain cycles, so a set of visited Promises is maintained. may https://codereview.chromium.org/2317383002/diff/220001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2317383002/diff/220001/src/parsing/parser.cc#... src/parsing/parser.cc:4741: // tmp = %AsyncFunctionAwait(.generator_object, tmp); Please update the comment.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2317383002/diff/80001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/80001/src/js/promise.js#newco... src/js/promise.js:290: if (!IS_UNDEFINED(resolution) && DEBUG_IS_ACTIVE && IsPromise(resolution)) { On 2016/09/08 at 21:38:58, gsathya wrote: > nit: format, s/DEBUG_IS_ACTIVE/instrumenting, and can you make instrumenting the first conditional on this? Done https://codereview.chromium.org/2317383002/diff/220001/src/js/collection.js File src/js/collection.js (right): https://codereview.chromium.org/2317383002/diff/220001/src/js/collection.js#n... src/js/collection.js:488: to.SetAdd = SetAdd; On 2016/09/12 at 13:22:00, jgruber wrote: > This may not be necessary (see comments at import sites). This is a common pattern, see ArrayToString, MapEntries, etc. This pattern avoids depending on the order that various files are included in the bootstrap. Although some such dependencies exist, I'd like to avoid creating too many new ones. https://codereview.chromium.org/2317383002/diff/220001/src/js/harmony-async-a... File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2317383002/diff/220001/src/js/harmony-async-a... src/js/harmony-async-await.js:83: throwawayCapability); On 2016/09/12 at 13:22:00, jgruber wrote: > Nit: needs formatting. Moved to one line https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js#newc... src/js/promise.js:33: var SetAdd; On 2016/09/12 at 13:22:00, jgruber wrote: > Could you do SetAdd = GlobalSet.prototype.add here instead of the export/import? I'd rather not, as explained in the other comment. https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js#newc... src/js/promise.js:585: // with another Promise, or an intermediate, hidden, throaway Promise On 2016/09/12 at 13:22:00, jgruber wrote: > throwaway Fixed https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js#newc... src/js/promise.js:590: if (outerPromise && On 2016/09/12 at 13:22:00, jgruber wrote: > Would it make sense to make this stricter using IsPromise(outerPromise)? That check would be pretty redundant, as we've already checked that it has this private symbol. What would the purpose be? https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js#newc... src/js/promise.js:619: // main contain cycles, so a set of visited Promises is maintained. On 2016/09/12 at 13:22:00, jgruber wrote: > may Fixed
https://codereview.chromium.org/2317383002/diff/220001/src/js/harmony-async-a... File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2317383002/diff/220001/src/js/harmony-async-a... src/js/harmony-async-await.js:85: if (debug_is_active) { On 2016/09/12 at 13:22:00, jgruber wrote: > Should this also check for !IS_UNDEFINED(outerPromise)? Maybe this block could be merged with the other promiseAwaitHandlerSymbol block above if it's moved until after NewPromiseCapability and before PerformPromiseThen. Added the IS_UNDEFINED check and merged the blocks. Seems valid as the dependency graph is not needed synchronously.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
Still lgtm modulo test failures. https://codereview.chromium.org/2317383002/diff/220001/src/js/collection.js File src/js/collection.js (right): https://codereview.chromium.org/2317383002/diff/220001/src/js/collection.js#n... src/js/collection.js:488: to.SetAdd = SetAdd; On 2016/09/12 22:49:25, Dan Ehrenberg wrote: > On 2016/09/12 at 13:22:00, jgruber wrote: > > This may not be necessary (see comments at import sites). > > This is a common pattern, see ArrayToString, MapEntries, etc. This pattern > avoids depending on the order that various files are included in the bootstrap. > Although some such dependencies exist, I'd like to avoid creating too many new > ones. Acknowledged. https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/220001/src/js/promise.js#newc... src/js/promise.js:590: if (outerPromise && On 2016/09/12 22:49:25, Dan Ehrenberg wrote: > On 2016/09/12 at 13:22:00, jgruber wrote: > > Would it make sense to make this stricter using IsPromise(outerPromise)? > > That check would be pretty redundant, as we've already checked that it has this > private symbol. What would the purpose be? For documenting and sanity-checking the type of outerPromise. But either way is fine for me.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2317383002/diff/260001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2317383002/diff/260001/src/parsing/parser.cc#... src/parsing/parser.cc:4411: zone()); This parser change should address the Blink failures. It preserves Yang's change to ensure that the right debugging positions are present. PTAL.
https://codereview.chromium.org/2317383002/diff/260001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2317383002/diff/260001/src/parsing/parser.cc#... src/parsing/parser.cc:4411: zone()); This parser change should address the Blink failures. It preserves Yang's change to ensure that the right debugging positions are present. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2317383002/diff/260001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2317383002/diff/260001/src/parsing/parser.cc#... src/parsing/parser.cc:4411: zone()); On 2016/09/14 01:09:14, Dan Ehrenberg wrote: > This parser change should address the Blink failures. It preserves Yang's change > to ensure that the right debugging positions are present. PTAL. Sorry, issues still remaining; will work on more tomorrow.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/12639) v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The newest patchset addresses previous issues; stack trace tests are marked NeedsManualRebaseline as this patch changes the results.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24115)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2317383002/diff/430001/src/js/harmony-async-a... File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2317383002/diff/430001/src/js/harmony-async-a... src/js/harmony-async-await.js:85: SET_PRIVATE(onRejected, promiseAwaitHandlerSymbol, outerPromise); Most other places that use promiseAwaitHandlerSymbol are set on Promise instances, but this sets it on a function. Is this case mentioned in your CL description? https://codereview.chromium.org/2317383002/diff/430001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/430001/src/js/promise.js#newc... src/js/promise.js:41: SetAdd = from.SetAdd; Nit: sort https://codereview.chromium.org/2317383002/diff/430001/src/js/promise.js#newc... src/js/promise.js:295: if (instrumenting && !IS_UNDEFINED(resolution) && No need to check !IS_UNDEFINED if you're checking IsPromise (which calls IS_RECEIVER). https://codereview.chromium.org/2317383002/diff/430001/src/js/promise.js#newc... src/js/promise.js:577: if (%_Call(SetHas, visited, promise)) return false; My brain is having trouble groking all the tests; are there tests covering this cycle-handling? https://codereview.chromium.org/2317383002/diff/430001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2317383002/diff/430001/src/parsing/parser.cc#... src/parsing/parser.cc:4399: Variable* promise_temp_var = Why do we need a new var here? Is something going to clobber .promise before we can "return" it from the do block? https://codereview.chromium.org/2317383002/diff/430001/src/parsing/parser.cc#... src/parsing/parser.cc:4405: factory()->NewExpressionStatement(promise_assignment, value->position()), Is it important that this uses the same position as the value assignment? That seems odd.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Rebased on top of a couple other fixes. This patch actually removes cycle detection, as it seems the cycles were only created as part of the whole leaky part that's now gone. I also simplified the dependency graph (edges from functions are different from edges from promises) and unified the handling of other Promise dependencies. PTAL https://codereview.chromium.org/2317383002/diff/430001/src/js/harmony-async-a... File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2317383002/diff/430001/src/js/harmony-async-a... src/js/harmony-async-await.js:85: SET_PRIVATE(onRejected, promiseAwaitHandlerSymbol, outerPromise); On 2016/09/15 at 22:39:40, adamk wrote: > Most other places that use promiseAwaitHandlerSymbol are set on Promise instances, but this sets it on a function. Is this case mentioned in your CL description? It's the first bullet. https://codereview.chromium.org/2317383002/diff/430001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/430001/src/js/promise.js#newc... src/js/promise.js:41: SetAdd = from.SetAdd; On 2016/09/15 at 22:39:40, adamk wrote: > Nit: sort Done https://codereview.chromium.org/2317383002/diff/430001/src/js/promise.js#newc... src/js/promise.js:295: if (instrumenting && !IS_UNDEFINED(resolution) && On 2016/09/15 at 22:39:40, adamk wrote: > No need to check !IS_UNDEFINED if you're checking IsPromise (which calls IS_RECEIVER). Done https://codereview.chromium.org/2317383002/diff/430001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2317383002/diff/430001/src/parsing/parser.cc#... src/parsing/parser.cc:4399: Variable* promise_temp_var = On 2016/09/15 at 22:39:40, adamk wrote: > Why do we need a new var here? Is something going to clobber .promise before we can "return" it from the do block? - To output something from a block, we need a Variable, not a VariableProxy, so we need to read it into something (since .promise is not resolved until scope analysis). - Even without that part, if I put a nopos variable read into an expression position as the third argument of AsyncFunctionAwait, it seems to affect the line number output (putting it at the beginning of the script). Reading .promise here seems to function as a workaround. https://codereview.chromium.org/2317383002/diff/430001/src/parsing/parser.cc#... src/parsing/parser.cc:4405: factory()->NewExpressionStatement(promise_assignment, value->position()), On 2016/09/15 at 22:39:40, adamk wrote: > Is it important that this uses the same position as the value assignment? That seems odd. It does not; changed it to nopos.
Rebased on top of a couple other fixes. This patch actually removes cycle detection, as it seems the cycles were only created as part of the whole leaky part that's now gone. I also simplified the dependency graph (edges from functions are different from edges from promises) and unified the handling of other Promise dependencies. PTAL https://codereview.chromium.org/2317383002/diff/430001/src/js/harmony-async-a... File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2317383002/diff/430001/src/js/harmony-async-a... src/js/harmony-async-await.js:85: SET_PRIVATE(onRejected, promiseAwaitHandlerSymbol, outerPromise); On 2016/09/15 at 22:39:40, adamk wrote: > Most other places that use promiseAwaitHandlerSymbol are set on Promise instances, but this sets it on a function. Is this case mentioned in your CL description? It's the first bullet. https://codereview.chromium.org/2317383002/diff/430001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/430001/src/js/promise.js#newc... src/js/promise.js:41: SetAdd = from.SetAdd; On 2016/09/15 at 22:39:40, adamk wrote: > Nit: sort Done https://codereview.chromium.org/2317383002/diff/430001/src/js/promise.js#newc... src/js/promise.js:295: if (instrumenting && !IS_UNDEFINED(resolution) && On 2016/09/15 at 22:39:40, adamk wrote: > No need to check !IS_UNDEFINED if you're checking IsPromise (which calls IS_RECEIVER). Done https://codereview.chromium.org/2317383002/diff/430001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2317383002/diff/430001/src/parsing/parser.cc#... src/parsing/parser.cc:4399: Variable* promise_temp_var = On 2016/09/15 at 22:39:40, adamk wrote: > Why do we need a new var here? Is something going to clobber .promise before we can "return" it from the do block? - To output something from a block, we need a Variable, not a VariableProxy, so we need to read it into something (since .promise is not resolved until scope analysis). - Even without that part, if I put a nopos variable read into an expression position as the third argument of AsyncFunctionAwait, it seems to affect the line number output (putting it at the beginning of the script). Reading .promise here seems to function as a workaround. https://codereview.chromium.org/2317383002/diff/430001/src/parsing/parser.cc#... src/parsing/parser.cc:4405: factory()->NewExpressionStatement(promise_assignment, value->position()), On 2016/09/15 at 22:39:40, adamk wrote: > Is it important that this uses the same position as the value assignment? That seems odd. It does not; changed it to nopos.
Thanks, the simplification lgtm, but I'd ideally like to get another look from the other reviewers given how much has changed since the last patch. https://codereview.chromium.org/2317383002/diff/530001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/530001/src/js/promise.js#newc... src/js/promise.js:505: let throwawayPromise = nextPromise.then( This is the first "let" in this file (or anywhere I can think of in natives); can you leave this as a var for consistency?
https://codereview.chromium.org/2317383002/diff/530001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/530001/src/js/promise.js#newc... src/js/promise.js:505: let throwawayPromise = nextPromise.then( On 2016/09/17 at 00:32:40, adamk wrote: > This is the first "let" in this file (or anywhere I can think of in natives); can you leave this as a var for consistency? Fixed
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2317383002/diff/550001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/550001/src/js/promise.js#newc... src/js/promise.js:511: SET_PRIVATE(throwawayPromise, promiseHandledBySymbol, deferred.promise); promiseHandledBySymbol is sometimes set only if we're debugging, sometimes unconditionally. promiseForwardingHandlerSymbol is always set, but used only during debugging (if my understanding is correct). Would it make sense to make this more consistent in a follow-up?
lgtm
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2317383002/#ps550001 (title: "let to var")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12992) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
https://codereview.chromium.org/2317383002/diff/550001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2317383002/diff/550001/src/js/promise.js#newc... src/js/promise.js:511: SET_PRIVATE(throwawayPromise, promiseHandledBySymbol, deferred.promise); On 2016/09/19 at 14:48:05, jgruber wrote: > promiseHandledBySymbol is sometimes set only if we're debugging, sometimes unconditionally. promiseForwardingHandlerSymbol is always set, but used only during debugging (if my understanding is correct). Would it make sense to make this more consistent in a follow-up? Rather than being based on symbols, it's more that Promise.all and Promise.race do some debugging-related things in non-debug mode, whereas async functions leave it for debug only. Will address in a follow-on patch.
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by littledan@chromium.org
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #30 (id:550001)
Message was sent while issue was closed.
Description was changed from ========== Async/await Promise dependency graph This patch knits together Promises returned by async/await such that when one async function awaits the result of another one, catch prediction works across the boundaries, whether the exception comes synchronously or asynchronously. Edges are added in three places: - When a locally uncaught await happens, if the value passed into await is a Promise, from the awaited value to the Promise under construction in the broader async function - From a "throwaway" Promise, which may be found on the Promise debug stack, to the Promise under construction in the async function that surrounds it - When a Promise is resolved with another Promise (e.g., when returning a Promise from an async function) BUG=v8:5167 ========== to ========== Async/await Promise dependency graph This patch knits together Promises returned by async/await such that when one async function awaits the result of another one, catch prediction works across the boundaries, whether the exception comes synchronously or asynchronously. Edges are added in three places: - When a locally uncaught await happens, if the value passed into await is a Promise, from the awaited value to the Promise under construction in the broader async function - From a "throwaway" Promise, which may be found on the Promise debug stack, to the Promise under construction in the async function that surrounds it - When a Promise is resolved with another Promise (e.g., when returning a Promise from an async function) BUG=v8:5167 Committed: https://crrev.com/7265fdde7c76b9f875b40b0b139515936d491d64 Cr-Commit-Position: refs/heads/master@{#39522} ==========
Message was sent while issue was closed.
Patchset 30 (id:??) landed as https://crrev.com/7265fdde7c76b9f875b40b0b139515936d491d64 Cr-Commit-Position: refs/heads/master@{#39522}
Message was sent while issue was closed.
A revert of this CL (patchset #30 id:550001) has been created in https://codereview.chromium.org/2351953002/ by littledan@chromium.org. The reason for reverting is: Need to break up test into smaller tests to avoid timeouts.
Message was sent while issue was closed.
On 2016/09/20 at 01:09:03, Dan Ehrenberg wrote: > A revert of this CL (patchset #30 id:550001) has been created in https://codereview.chromium.org/2351953002/ by littledan@chromium.org. > > The reason for reverting is: Need to break up test into smaller tests to avoid timeouts. Reland in https://codereview.chromium.org/2346363004 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
