Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2233923003:
Desugar async/await to create the resulting Promise upfront (Closed)
|
Created:
4 years, 4 months ago by Dan Ehrenberg Modified:
4 years, 3 months ago CC:
gsathya, jgruber, v8-reviews_googlegroups.com, Yang Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionDesugar async/await to create the resulting Promise upfront
As part of the work to implement catch prediction for async functions,
the resulting Promise that is the output of the function needs to be
available earlier for a couple reasons:
- To be able to do %DebugPushPromise/%DebugPopPromise over the body
of the async function
- To be able to pass the resulting promise into AsyncFunctionAwait
in order to set up the dependency chains
This patch creates the Promise earlier and pushes it onto the debug
stack; a later patch will set up the dependency chain. Although the
debug stack is set up, it's not anticipated that this will change
the catch prediction helpfully yet, as everything will still likely
be predicted as 'caught' for now, as before.
R=caitp@igalia.com,yangguo@chromium.org
CC=neis@chromium.org,gsathya@chromium.org
BUG=v8:5167
Committed: https://crrev.com/5386c0062d8ae6ab6975a6f5071fae628377e7b8
Cr-Commit-Position: refs/heads/master@{#38957}
Patch Set 1 #Patch Set 2 : git cl format #Patch Set 3 : Declare correctly for rebase #Patch Set 4 : Check for stack overflow on function length getter #
Total comments: 2
Patch Set 5 : Better approach for bailing out on stack overflow in the appropriate place in fullcodegen #
Total comments: 2
Patch Set 6 : Rebase on top of exception events patch #Patch Set 7 : rebase #Patch Set 8 : rebase #Patch Set 9 : Fix polarity of async function rejections #
Total comments: 25
Patch Set 10 : Changes from review #
Total comments: 2
Patch Set 11 : Rebase #Patch Set 12 : format #
Dependent Patchsets: Messages
Total messages: 82 (63 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/21584)
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_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) 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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10784) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) 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_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) 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...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6794) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
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_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
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_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/12365) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_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 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/2233923003/diff/60001/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/2233923003/diff/60001/src/accessors.cc#newcod... src/accessors.cc:744: result = handle(Smi::FromInt(0), isolate); thinking about this, we only want to return 0 if compiling is actually needed, right? Are we not able to move this to JSFunction::GetLength()?
https://codereview.chromium.org/2233923003/diff/60001/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/2233923003/diff/60001/src/accessors.cc#newcod... src/accessors.cc:744: result = handle(Smi::FromInt(0), isolate); On 2016/08/12 at 01:57:10, caitp wrote: > thinking about this, we only want to return 0 if compiling is actually needed, right? Are we not able to move this to JSFunction::GetLength()? I backed out this change in the most recent version. Now, it'll throw an exception and go down the exception path because of the stack overflow from within fullcodegen.
littledan@chromium.org changed reviewers: + neis@chromium.org
littledan@chromium.org changed reviewers: + adamk@chromium.org
PTAL
adamk@chromium.org changed reviewers: + bmeurer@chromium.org - yangguo@chromium.org
I'd like someone more familiar with the async/await implementation to review the desugaring (neis + caitp?), but it also looks like we'll need a different full-codegen OWNER with Yang out; adding bmeurer. https://codereview.chromium.org/2233923003/diff/80001/src/full-codegen/full-c... File src/full-codegen/full-codegen.cc (right): https://codereview.chromium.org/2233923003/diff/80001/src/full-codegen/full-c... src/full-codegen/full-codegen.cc:899: if (HasStackOverflow()) return; These additional HasStackOverflow() are surprising, since there's currently only a single explicit check for this in full-codegen.cc right now. Why did they get added?
https://codereview.chromium.org/2233923003/diff/80001/src/full-codegen/full-c... File src/full-codegen/full-codegen.cc (right): https://codereview.chromium.org/2233923003/diff/80001/src/full-codegen/full-c... src/full-codegen/full-codegen.cc:899: if (HasStackOverflow()) return; On 2016/08/22 at 18:47:39, adamk wrote: > These additional HasStackOverflow() are surprising, since there's currently only a single explicit check for this in full-codegen.cc right now. Why did they get added? The new desugaring ends up making some significant finally blocks. It turns out that finally blocks trigger recursion in fullcodegen in a different way from what the AstVisitor is already testing for. As a result, leaving out these checks led to corruption in fullcodegen's datastructures. I only needed one of these (IIRC EmitUnwindAndReturn) but it seemed reasonable to add the others by analogy, even if we don't currently have a desugaring that would lead to an overflow in this case.
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_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
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...
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_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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Desugar async/await to create the resulting Promise upfront As part of the work to implement catch prediction for async functions, the resulting Promise that is the output of the function needs to be available earlier for a couple reasons: - To be able to do %DebugPushPromise/%DebugPopPromise over the body of the async function - To be able to pass the resulting promise into AsyncFunctionAwait in order to set up the dependency chains This patch creates the Promise earlier and pushes it onto the debug stack; a later patch will set up the dependency chain. Although the debug stack is set up, it's not anticipated that this will change the catch prediction helpfully yet, as everything will still likely be predicted as 'caught' for now, as before. R=caitp@igalia.com,yangguo@chromium.org CC=neis@chromium.org,gsathya@chromium.org BUG=v8:5167 ========== to ========== Desugar async/await to create the resulting Promise upfront As part of the work to implement catch prediction for async functions, the resulting Promise that is the output of the function needs to be available earlier for a couple reasons: - To be able to do %DebugPushPromise/%DebugPopPromise over the body of the async function - To be able to pass the resulting promise into AsyncFunctionAwait in order to set up the dependency chains This patch creates the Promise earlier and pushes it onto the debug stack; a later patch will set up the dependency chain. Although the debug stack is set up, it's not anticipated that this will change the catch prediction helpfully yet, as everything will still likely be predicted as 'caught' for now, as before. R=caitp@igalia.com,yangguo@chromium.org CC=neis@chromium.org,gsathya@chromium.org BUG=v8:5167 ==========
jgruber@chromium.org changed reviewers: + jgruber@chromium.org
A couple of minor comments from my side: https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4577: Block* block = factory()->NewBlock(nullptr, 1, true, kNoSourcePosition); We could set the initial size to 3 to avoid having to grow the ZoneList. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4582: DeclareVariable(ast_value_factory()->dot_promise_string(), VAR, Since I'm not familiar with the parser, a question: Whenever I see dot_* names used for variables in the parser, NewTemporary() is used instead of DeclareVariable(). Is that not applicable here? https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4594: // var .debug_is_active = %_DebugIsActive() != 0; Nit: This differs from !== above. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4701: ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(3, zone()); Should this be 2? There's only two args added. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4714: VariableProxy* Parser::BuildDebugIsActive() { This should be BuildDotDebugIsActive for consistency.
https://codereview.chromium.org/2233923003/diff/160001/src/js/harmony-async-a... File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2233923003/diff/160001/src/js/harmony-async-a... src/js/harmony-async-await.js:59: "promise_reject_no_debug_event", RejectPromiseNoDebugEvent, reject_promise_... https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc File src/parsing/parser.cc (left): https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:3969: // .generator_object = %CreateGeneratorObject(); The .generator_object assignment is actually outside the try block. Can you fix the comment? https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:3986: Block* try_block = factory()->NewBlock(NULL, 8, true, kNoSourcePosition); This shouldn't be called try_block anymore. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:3988: ZoneList<Statement*>* inner_body = try_block->statements(); Can you get rid of inner_body and just inline it? While reading this code I got confused several times by thinking inner_body is something else. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4565: Block* Parser::BuildRejectPromiseOnException(Block* inner_block, bool* ok) { s/inner_block/block/ to match the comment. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4567: // var .debug_is_active = %_DebugIsActive() !== 0; I think we don't need the !== 0 comparison. (But why doesn't DebugIsActive return a boolean in the first place?) https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4569: // if (.debug_is_active) %DebugPushPromise(.promise); Can't we move this out of the try-block? Seems cleaner and avoids creating another block. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4577: Block* block = factory()->NewBlock(nullptr, 1, true, kNoSourcePosition); s/block/result/ for clarity (and to avoid conflict) https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4700: // since a debug event already happened for the exception that got us here. I think you mean 'false' here, not 'true', but since it doesn't appear in the AST anymore you don't need to mention the argument. I'm wondering, though, if it's really worth having RejectPromiseNoDebugEvent as a seperate definition.
Thanks for the review comments; made the appropriate cleanups. PTAL. https://codereview.chromium.org/2233923003/diff/160001/src/js/harmony-async-a... File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2233923003/diff/160001/src/js/harmony-async-a... src/js/harmony-async-await.js:59: "promise_reject_no_debug_event", RejectPromiseNoDebugEvent, On 2016/08/24 at 09:48:51, neis wrote: > reject_promise_... Fixed https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc File src/parsing/parser.cc (left): https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:3969: // .generator_object = %CreateGeneratorObject(); On 2016/08/24 at 09:48:51, neis wrote: > The .generator_object assignment is actually outside the try block. Can you fix the comment? Fixed https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4565: Block* Parser::BuildRejectPromiseOnException(Block* inner_block, bool* ok) { On 2016/08/24 at 09:48:51, neis wrote: > s/inner_block/block/ to match the comment. Changed the name in the other direction https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4567: // var .debug_is_active = %_DebugIsActive() !== 0; On 2016/08/24 at 09:48:51, neis wrote: > I think we don't need the !== 0 comparison. (But why doesn't DebugIsActive return a boolean in the first place?) Right, I was considering this but left it out as the existing Promise code did the comparison. But I don't see the point either, so the new patch removes the comparison. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4569: // if (.debug_is_active) %DebugPushPromise(.promise); On 2016/08/24 at 09:48:51, neis wrote: > Can't we move this out of the try-block? Seems cleaner and avoids creating another block. Done. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4577: Block* block = factory()->NewBlock(nullptr, 1, true, kNoSourcePosition); On 2016/08/24 at 09:48:51, neis wrote: > s/block/result/ for clarity (and to avoid conflict) Done, and fixed the initial size to 4. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4582: DeclareVariable(ast_value_factory()->dot_promise_string(), VAR, On 2016/08/24 at 09:16:36, jgruber wrote: > Since I'm not familiar with the parser, a question: Whenever I see dot_* names used for variables in the parser, NewTemporary() is used instead of DeclareVariable(). Is that not applicable here? I could do that, but then I'd have to store the temporary in FunctionState like .generator_object is. I don't really see the point in going through that bookkeeping in this case--scope analysis does just fine here. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4594: // var .debug_is_active = %_DebugIsActive() != 0; On 2016/08/24 at 09:16:36, jgruber wrote: > Nit: This differs from !== above. Removed the comparison, as it is unnecessary. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4700: // since a debug event already happened for the exception that got us here. On 2016/08/24 at 09:48:51, neis wrote: > I think you mean 'false' here, not 'true', but since it doesn't appear in the AST anymore you don't need to mention the argument. I'm wondering, though, if it's really worth having RejectPromiseNoDebugEvent as a seperate definition. I made it a separate function because RejectPromise is exported to a couple other places and I didn't want to mess with it, especially once the calling convention switched to using 'true' to trigger an event (which pre-existing callers did). Fixed the comment. https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4701: ZoneList<Expression*>* args = new (zone()) ZoneList<Expression*>(3, zone()); On 2016/08/24 at 09:16:36, jgruber wrote: > Should this be 2? There's only two args added. Fixed https://codereview.chromium.org/2233923003/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:4714: VariableProxy* Parser::BuildDebugIsActive() { On 2016/08/24 at 09:16:36, jgruber wrote: > This should be BuildDotDebugIsActive for consistency. Done
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/22511)
lgtm modulo one comment. https://codereview.chromium.org/2233923003/diff/180001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2233923003/diff/180001/src/parsing/parser.cc#... src/parsing/parser.cc:4462: DeclareVariable(ast_value_factory()->dot_promise_string(), VAR, Please declare this CONST, not VAR.
lgtm
https://codereview.chromium.org/2233923003/diff/180001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2233923003/diff/180001/src/parsing/parser.cc#... src/parsing/parser.cc:4462: DeclareVariable(ast_value_factory()->dot_promise_string(), VAR, On 2016/08/25 at 08:15:22, neis wrote: > Please declare this CONST, not VAR. I use VAR rather than CONST here so that if there end up being two such blocks (which can happen if you have non-simple parameters) then the repeated write to .promise does not cause an error. Another way would be to create a separate inner scope, but I thought that this would be simpler, and it avoids the overhead of hole checks. WDYT?
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/22610)
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.
On 2016/08/25 14:23:34, Dan Ehrenberg wrote: > https://codereview.chromium.org/2233923003/diff/180001/src/parsing/parser.cc > File src/parsing/parser.cc (right): > > https://codereview.chromium.org/2233923003/diff/180001/src/parsing/parser.cc#... > src/parsing/parser.cc:4462: > DeclareVariable(ast_value_factory()->dot_promise_string(), VAR, > On 2016/08/25 at 08:15:22, neis wrote: > > Please declare this CONST, not VAR. > > I use VAR rather than CONST here so that if there end up being two such blocks > (which can happen if you have non-simple parameters) then the repeated write to > .promise does not cause an error. Another way would be to create a separate > inner scope, but I thought that this would be simpler, and it avoids the > overhead of hole checks. WDYT? I understand now that this is related to the TODO about merging the rejection blocks. I'm fine with landing as is, because addressing the TODO is not trivial (but it should definitely happen).
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org, jgruber@chromium.org Link to the patchset: https://codereview.chromium.org/2233923003/#ps220001 (title: "format")
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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22649)
LGTM on fullcodegen.
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.
Description was changed from ========== Desugar async/await to create the resulting Promise upfront As part of the work to implement catch prediction for async functions, the resulting Promise that is the output of the function needs to be available earlier for a couple reasons: - To be able to do %DebugPushPromise/%DebugPopPromise over the body of the async function - To be able to pass the resulting promise into AsyncFunctionAwait in order to set up the dependency chains This patch creates the Promise earlier and pushes it onto the debug stack; a later patch will set up the dependency chain. Although the debug stack is set up, it's not anticipated that this will change the catch prediction helpfully yet, as everything will still likely be predicted as 'caught' for now, as before. R=caitp@igalia.com,yangguo@chromium.org CC=neis@chromium.org,gsathya@chromium.org BUG=v8:5167 ========== to ========== Desugar async/await to create the resulting Promise upfront As part of the work to implement catch prediction for async functions, the resulting Promise that is the output of the function needs to be available earlier for a couple reasons: - To be able to do %DebugPushPromise/%DebugPopPromise over the body of the async function - To be able to pass the resulting promise into AsyncFunctionAwait in order to set up the dependency chains This patch creates the Promise earlier and pushes it onto the debug stack; a later patch will set up the dependency chain. Although the debug stack is set up, it's not anticipated that this will change the catch prediction helpfully yet, as everything will still likely be predicted as 'caught' for now, as before. R=caitp@igalia.com,yangguo@chromium.org CC=neis@chromium.org,gsathya@chromium.org BUG=v8:5167 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Desugar async/await to create the resulting Promise upfront As part of the work to implement catch prediction for async functions, the resulting Promise that is the output of the function needs to be available earlier for a couple reasons: - To be able to do %DebugPushPromise/%DebugPopPromise over the body of the async function - To be able to pass the resulting promise into AsyncFunctionAwait in order to set up the dependency chains This patch creates the Promise earlier and pushes it onto the debug stack; a later patch will set up the dependency chain. Although the debug stack is set up, it's not anticipated that this will change the catch prediction helpfully yet, as everything will still likely be predicted as 'caught' for now, as before. R=caitp@igalia.com,yangguo@chromium.org CC=neis@chromium.org,gsathya@chromium.org BUG=v8:5167 ========== to ========== Desugar async/await to create the resulting Promise upfront As part of the work to implement catch prediction for async functions, the resulting Promise that is the output of the function needs to be available earlier for a couple reasons: - To be able to do %DebugPushPromise/%DebugPopPromise over the body of the async function - To be able to pass the resulting promise into AsyncFunctionAwait in order to set up the dependency chains This patch creates the Promise earlier and pushes it onto the debug stack; a later patch will set up the dependency chain. Although the debug stack is set up, it's not anticipated that this will change the catch prediction helpfully yet, as everything will still likely be predicted as 'caught' for now, as before. R=caitp@igalia.com,yangguo@chromium.org CC=neis@chromium.org,gsathya@chromium.org BUG=v8:5167 Committed: https://crrev.com/5386c0062d8ae6ab6975a6f5071fae628377e7b8 Cr-Commit-Position: refs/heads/master@{#38957} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/5386c0062d8ae6ab6975a6f5071fae628377e7b8 Cr-Commit-Position: refs/heads/master@{#38957} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
