Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2685683002:
[async-await] (simpler) fix for Return in try/finally in async functions (Closed)
Description[async-await] (simpler) fix for Return in try/finally in async functions
Alternative approach to https://codereview.chromium.org/2667983004/, which
does not depend on implicit control flow changes from
https://codereview.chromium.org/2664083002
- Remove handling for `async function` from Parser::RewriteReturn(). This functionality
is moved to BytecodeGenerator::BuildAsyncReturn(). This ensures that promise resolution
is deferred until all finally blocks are evaluated fully.
- Add a new deferred command (CMD_ASYNC_RETURN), which instructs ControlScope to
generate return code using BuildAsyncReturn rather than BuildReturn.
- Parser has a new `NewReturnStatement()` helper which determines what type of return
statement to generate based on the type of function.
BUG=v8:5896, v8:4483
R=littledan@chromium.org, neis@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, gsathya@chromium.org
Review-Url: https://codereview.chromium.org/2685683002
Cr-Commit-Position: refs/heads/master@{#43104}
Committed: https://chromium.googlesource.com/v8/v8/+/39642fa2bef2ca151af32fa0f5bfbf357e8914aa
Patch Set 1 #
Total comments: 4
Patch Set 2 : add some comments + cleanup using BuildHardReturn #
Total comments: 9
Patch Set 3 : Make a new deferred command type, instead of overloading BuildReturn. #Patch Set 4 : remove comment #
Total comments: 2
Patch Set 5 : [async-await] (simpler) fix for Return in try/finally in async functions #
Total comments: 15
Patch Set 6 : rename ParserBase::NewReturnStatement to ParserBase::BuildReturnStatement() #Patch Set 7 : add a "RareData" thing to shrink memory use #
Total comments: 1
Patch Set 8 : -Wunused-variable again D: #Patch Set 9 : also shrink the bitfield for ReturnStatement::Type #
Messages
Total messages: 72 (30 generated)
The CQ bit was checked by caitp@igalia.com 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...
here's a version which doesn't move the implicit flow control to BCG. The implicit flow-control in BCG solves a subtle problem which this CL introduces another mechanism to address, but it's sort of hack-ish. I obviously think it's better if we do go with the option of doing implicit control flow in BCG, and am of course still happy to fix the issue with named-function-expressions potentially falling into Ignition when they otherwise wouldn't have.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Interpreter parts LGTM with a couple of nits. Thanks. https://codereview.chromium.org/2685683002/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2685683002/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:709: DCHECK(execution_control() == &control); nit - please add a comment here that we can do a hard return since we know that the execution_control is the top level and doesn't need to defer the return. https://codereview.chromium.org/2685683002/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:1068: builder()->Return(); BuildTraceExit here too? How about Creating a BuildHardReturn() function which does the trace exit and builder()->Return and call it here, in the implicit return and the BuildReturn call (in which case, you probably wouldn't need the extra BuildTraceExit function).
The CQ bit was checked by caitp@igalia.com 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...
Alright, if people aren't too uncomfortable with the hack in the ReturnStatement AST node, this might be the way to go to fix this (and then I can get back to work on for-await-of :)) https://codereview.chromium.org/2685683002/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2685683002/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:709: DCHECK(execution_control() == &control); On 2017/02/08 12:21:56, rmcilroy wrote: > nit - please add a comment here that we can do a hard return since we know that > the execution_control is the top level and doesn't need to defer the return. Done. https://codereview.chromium.org/2685683002/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:1068: builder()->Return(); On 2017/02/08 12:21:56, rmcilroy wrote: > BuildTraceExit here too? How about Creating a BuildHardReturn() function which > does the trace exit and builder()->Return and call it here, in the implicit > return and the BuildReturn call (in which case, you probably wouldn't need the > extra BuildTraceExit function). That looks cleaner, nice. Done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hey, I like where this patch is going--it does seem cleaner to put those variables in the DeclarationScope, and I like the idea of desugaring the return statement for async functions in the interpreter. What I don't understand is what this patch changes about the desugaring. It looks like it desugars to the same thing as before... what am I missing? (Maybe the answer should go in the commit message) https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h#newcode899 src/ast/ast.h:899: enum Type { kNormal, kHardReturn }; Bikeshedding suggestion: Instead, have normal returns and async returns. RewriteReturn would turn return statements into async return statements when in an async function. RewriteReturn only runs for syntactic return statements, not for ones that come out of desugaring, so this should end up doing the same thing, right? https://codereview.chromium.org/2685683002/diff/20001/src/ast/scopes.h File src/ast/scopes.h (right): https://codereview.chromium.org/2685683002/diff/20001/src/ast/scopes.h#newcod... src/ast/scopes.h:897: Variable* promise_; Will there be more live DeclarationScopes than FunctionStates at a time? If so, moving the variables this way this might cause a (probably minor) increase in memory usage.
On 2017/02/08 21:12:56, Dan Ehrenberg wrote: > Hey, I like where this patch is going--it does seem cleaner to put those > variables in the DeclarationScope, and I like the idea of desugaring the return > statement for async functions in the interpreter. > > What I don't understand is what this patch changes about the desugaring. It > looks like it desugars to the same thing as before... what am I missing? (Maybe > the answer should go in the commit message) The parser still does adds its try/catch blocks. I will clean that up later, once --turbo is enabled by default. The key difference is that for normal ReturnStatement nodes, BCG will turn those into `ResolvePromise(<function_promise>)` calls, rather than doing that in Parser::RewriteReturn(). This means normal return calls can take advantage of the ControlScope state machine, which deals with recording a return variable when a finally scope will be hit, and deferring the actual return (in BCG::BuildReturn()) until later on. Because the parser still adds try/catch blocks, and rejects and returns the function Promise if it sees an exception, the return statement is changed to a "hard return". The hard-return'd value doesn't get passed as an argument to ResolvePromise() in BCG. This prevents cyclic promise-resolution errors in an unobservable (I think) way (examples of how it could have been observed are shown in the CL description). Basically, the AST node change is just a hack to avoid doing the implicit control flow in BCG, though I still want to do that in BCG later on. > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h > File src/ast/ast.h (right): > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h#newcode899 > src/ast/ast.h:899: enum Type { kNormal, kHardReturn }; > Bikeshedding suggestion: Instead, have normal returns and async returns. > RewriteReturn would turn return statements into async return statements when in > an async function. RewriteReturn only runs for syntactic return statements, not > for ones that come out of desugaring, so this should end up doing the same > thing, right? > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/scopes.h > File src/ast/scopes.h (right): > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/scopes.h#newcod... > src/ast/scopes.h:897: Variable* promise_; > Will there be more live DeclarationScopes than FunctionStates at a time? If so, > moving the variables this way this might cause a (probably minor) increase in > memory usage. I believe the vast majority of allocated scopes are block-sxopes, so I want to say that the difference is probably very small, but I don't know. If it does end up being a not insignificant difference, I have some ideas for solving it.
On 2017/02/08 21:43:27, caitp wrote: > On 2017/02/08 21:12:56, Dan Ehrenberg wrote: > > Hey, I like where this patch is going--it does seem cleaner to put those > > variables in the DeclarationScope, and I like the idea of desugaring the > return > > statement for async functions in the interpreter. > > > > What I don't understand is what this patch changes about the desugaring. It > > looks like it desugars to the same thing as before... what am I missing? > (Maybe > > the answer should go in the commit message) > > The parser still does adds its try/catch blocks. I will clean that up later, > once --turbo is enabled by default. > > The key difference is that for normal ReturnStatement nodes, BCG will turn those > into > `ResolvePromise(<function_promise>)` calls, rather than doing that in > Parser::RewriteReturn(). This means > normal return calls can take advantage of the ControlScope state machine, which > deals with recording a > return variable when a finally scope will be hit, and deferring the actual > return (in BCG::BuildReturn()) until > later on. > > Because the parser still adds try/catch blocks, and rejects and returns the > function Promise if it sees an > exception, the return statement is changed to a "hard return". The hard-return'd > value doesn't get passed as an > argument to ResolvePromise() in BCG. This prevents cyclic promise-resolution > errors in an unobservable (I think) > way (examples of how it could have been observed are shown in the CL > description). > > Basically, the AST node change is just a hack to avoid doing the implicit > control flow in BCG, though I still want > to do that in BCG later on. I don't understand how the code here affects this "circular resolution" issue. We didn't have that bug before the patch. Anyway, even if it weren't for the Promise code recognizing that as circular, I'd categorize this as "returning the wrong promise"--the async/await semantics are clear that a new Promise is returned, and this patch is good to not introduce that sort of bug. I'm still at a loss for how this desugaring change (which seems basically valid to me) addresses the bug. What is it doing to interact with the ControlScope/execution_control machinery? I don't see it referenced except in a DCHECK. > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h > > File src/ast/ast.h (right): > > > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h#newcode899 > > src/ast/ast.h:899: enum Type { kNormal, kHardReturn }; > > Bikeshedding suggestion: Instead, have normal returns and async returns. > > RewriteReturn would turn return statements into async return statements when > in > > an async function. RewriteReturn only runs for syntactic return statements, > not > > for ones that come out of desugaring, so this should end up doing the same > > thing, right? Do you think this is a bad idea? > > > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/scopes.h > > File src/ast/scopes.h (right): > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/scopes.h#newcod... > > src/ast/scopes.h:897: Variable* promise_; > > Will there be more live DeclarationScopes than FunctionStates at a time? If > so, > > moving the variables this way this might cause a (probably minor) increase in > > memory usage. > > I believe the vast majority of allocated scopes are block-sxopes, so I want to > say that the difference is probably very small, > but I don't know. If it does end up being a not insignificant difference, I have > some ideas for solving it.
https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h#newcode899 src/ast/ast.h:899: enum Type { kNormal, kHardReturn }; On 2017/02/08 21:12:56, Dan Ehrenberg wrote: > Bikeshedding suggestion: Instead, have normal returns and async returns. > RewriteReturn would turn return statements into async return statements when in > an async function. RewriteReturn only runs for syntactic return statements, not > for ones that come out of desugaring, so this should end up doing the same > thing, right? As explained on IRC: The key fix for the try/finally issue is to remove the Parser::RewriteReturn() stuff for async functions. In place of calling `ResolvePromise(<function_promise>, return_value);` in the Parser/AST, it's done in BCG::BuildReturn(). But, because this version doesn't move the implicit control flow (top-level try/catch blocks) into BCG, and the parser still generates the `return RejectPromise(). <function_promise>;`, or effectively `return <function_promise>;` at the end of those implicit try/catch blocks. As noted above, BCG would turn this into `ResolvePromise(<function_promise>, <function_promise>)`, which is a circular promise resolution. This "hard return" bit tells the BCG not to transform the return value into a ResolvePromise() call, so it can avoid the circular promise resolution. Please let me know if I'm not explaining this clearly.
https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:713: BuildHardReturn(); Why is this a hard return? I would expect implicit returns to resolve the promise with 'undefined', rather than actually return 'undefined'.
https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h#newcode899 src/ast/ast.h:899: enum Type { kNormal, kHardReturn }; On 2017/02/08 23:22:15, caitp wrote: > On 2017/02/08 21:12:56, Dan Ehrenberg wrote: > > Bikeshedding suggestion: Instead, have normal returns and async returns. > > RewriteReturn would turn return statements into async return statements when > in > > an async function. RewriteReturn only runs for syntactic return statements, > not > > for ones that come out of desugaring, so this should end up doing the same > > thing, right? > > As explained on IRC: > > The key fix for the try/finally issue is to remove the Parser::RewriteReturn() > stuff for async functions. > > In place of calling `ResolvePromise(<function_promise>, return_value);` in the > Parser/AST, it's done in BCG::BuildReturn(). > > But, because this version doesn't move the implicit control flow (top-level > try/catch blocks) into BCG, and the parser still generates the `return > RejectPromise(). <function_promise>;`, or effectively `return > <function_promise>;` at the end of those implicit try/catch blocks. As noted > above, BCG would turn this into `ResolvePromise(<function_promise>, > <function_promise>)`, which is a circular promise resolution. > > This "hard return" bit tells the BCG not to transform the return value into a > ResolvePromise() call, so it can avoid the circular promise resolution. > > Please let me know if I'm not explaining this clearly. I see how you need to differentiate between returns that need the promise behavior and returns that don't in order to avoid introducing a bug, and the logic seems valid here. Above, I was making a style suggestion about phrasing that logic--I though the logic I described would end up generating the same bytecode. I am still missing something about how this fixes the issue. You end up desugaring (in the BCG now) to the same thing in some cases. Which cases differ? I can't find them. Is this all about the implicit 'return undefined' generated at the end?
On 2017/02/09 06:46:58, Dan Ehrenberg wrote: > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h > File src/ast/ast.h (right): > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h#newcode899 > src/ast/ast.h:899: enum Type { kNormal, kHardReturn }; > On 2017/02/08 23:22:15, caitp wrote: > > On 2017/02/08 21:12:56, Dan Ehrenberg wrote: > > > Bikeshedding suggestion: Instead, have normal returns and async returns. > > > RewriteReturn would turn return statements into async return statements when > > in > > > an async function. RewriteReturn only runs for syntactic return statements, > > not > > > for ones that come out of desugaring, so this should end up doing the same > > > thing, right? > > > > As explained on IRC: > > > > The key fix for the try/finally issue is to remove the Parser::RewriteReturn() > > stuff for async functions. > > > > In place of calling `ResolvePromise(<function_promise>, return_value);` in the > > Parser/AST, it's done in BCG::BuildReturn(). > > > > But, because this version doesn't move the implicit control flow (top-level > > try/catch blocks) into BCG, and the parser still generates the `return > > RejectPromise(). <function_promise>;`, or effectively `return > > <function_promise>;` at the end of those implicit try/catch blocks. As noted > > above, BCG would turn this into `ResolvePromise(<function_promise>, > > <function_promise>)`, which is a circular promise resolution. > > > > This "hard return" bit tells the BCG not to transform the return value into a > > ResolvePromise() call, so it can avoid the circular promise resolution. > > > > Please let me know if I'm not explaining this clearly. > > I see how you need to differentiate between returns that need the promise > behavior and returns that don't in order to avoid introducing a bug, and the > logic seems valid here. Above, I was making a style suggestion about phrasing > that logic--I though the logic I described would end up generating the same > bytecode. > > I am still missing something about how this fixes the issue. You end up > desugaring (in the BCG now) to the same thing in some cases. Which cases differ? > I can't find them. Is this all about the implicit 'return undefined' generated > at the end? It only changes the way ReturnStatements in authored code work, by making them work with the normal delegates command system for try/finally. The returns inserted by the parser don't change, and use the hard-return mechanic to prevent them from changing. So, yeah, it only affects actual return statements written in the source text.
https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:713: BuildHardReturn(); On 2017/02/09 06:40:00, Dan Ehrenberg wrote: > Why is this a hard return? I would expect implicit returns to resolve the > promise with 'undefined', rather than actually return 'undefined'. This change is just too shrink bytecode a bit. The parser inserts an actual "implicit return" as the last statement in the try block, so this thing is actually unreachable. Even so, all blocks need a return, so this generates "LdaUndefined Return" instead of the full promise resolution thing, because for async functions and generators it doesn't actually matter.
On 2017/02/09 12:49:42, caitp wrote: > On 2017/02/09 06:46:58, Dan Ehrenberg wrote: > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h > > File src/ast/ast.h (right): > > > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h#newcode899 > > src/ast/ast.h:899: enum Type { kNormal, kHardReturn }; > > On 2017/02/08 23:22:15, caitp wrote: > > > On 2017/02/08 21:12:56, Dan Ehrenberg wrote: > > > > Bikeshedding suggestion: Instead, have normal returns and async returns. > > > > RewriteReturn would turn return statements into async return statements > when > > > in > > > > an async function. RewriteReturn only runs for syntactic return > statements, > > > not > > > > for ones that come out of desugaring, so this should end up doing the same > > > > thing, right? > > > > > > As explained on IRC: > > > > > > The key fix for the try/finally issue is to remove the > Parser::RewriteReturn() > > > stuff for async functions. > > > > > > In place of calling `ResolvePromise(<function_promise>, return_value);` in > the > > > Parser/AST, it's done in BCG::BuildReturn(). > > > > > > But, because this version doesn't move the implicit control flow (top-level > > > try/catch blocks) into BCG, and the parser still generates the `return > > > RejectPromise(). <function_promise>;`, or effectively `return > > > <function_promise>;` at the end of those implicit try/catch blocks. As noted > > > above, BCG would turn this into `ResolvePromise(<function_promise>, > > > <function_promise>)`, which is a circular promise resolution. > > > > > > This "hard return" bit tells the BCG not to transform the return value into > a > > > ResolvePromise() call, so it can avoid the circular promise resolution. > > > > > > Please let me know if I'm not explaining this clearly. > > > > I see how you need to differentiate between returns that need the promise > > behavior and returns that don't in order to avoid introducing a bug, and the > > logic seems valid here. Above, I was making a style suggestion about phrasing > > that logic--I though the logic I described would end up generating the same > > bytecode. > > > > I am still missing something about how this fixes the issue. You end up > > desugaring (in the BCG now) to the same thing in some cases. Which cases > differ? > > I can't find them. Is this all about the implicit 'return undefined' generated > > at the end? > > It only changes the way ReturnStatements in authored code work, by making them > work with the normal delegates command system for try/finally. The returns > inserted by the parser don't change, and use the hard-return mechanic to prevent > them from changing. > > So, yeah, it only affects actual return statements written in the source text. "by making them work with the normal delegates command system for try/finally." OK, I guess this is the part I don't understand. What's this interaction here, and why wasn't it working before?
https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:713: BuildHardReturn(); On 2017/02/09 12:53:07, caitp wrote: > On 2017/02/09 06:40:00, Dan Ehrenberg wrote: > > Why is this a hard return? I would expect implicit returns to resolve the > > promise with 'undefined', rather than actually return 'undefined'. > > This change is just too shrink bytecode a bit. > > The parser inserts an actual "implicit return" as the last statement in the try > block, so this thing is actually unreachable. Even so, all blocks need a return, > so this generates "LdaUndefined Return" instead of the full promise resolution > thing, because for async functions and generators it doesn't actually matter. I thought this would be reached in a case like "async function() { }". How does that end up resolving the Promise with undefined?
https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:713: BuildHardReturn(); On 2017/02/09 13:23:36, Dan Ehrenberg wrote: > On 2017/02/09 12:53:07, caitp wrote: > > On 2017/02/09 06:40:00, Dan Ehrenberg wrote: > > > Why is this a hard return? I would expect implicit returns to resolve the > > > promise with 'undefined', rather than actually return 'undefined'. > > > > This change is just too shrink bytecode a bit. > > > > The parser inserts an actual "implicit return" as the last statement in the > try > > block, so this thing is actually unreachable. Even so, all blocks need a > return, > > so this generates "LdaUndefined Return" instead of the full promise resolution > > thing, because for async functions and generators it doesn't actually matter. > > I thought this would be reached in a case like "async function() { }". How does > that end up resolving the Promise with undefined? I didn't realize this was only a to optimize bytecode size, I thought it was necessary with the promise resolution. I'm a bit less happy with having this as a hard return just because it's dead code in async code. Could we just make it a normal return for now? Going forward, if the parser is smart enough to add implicit returns for async functions, could it add implicit returns for all functions? If so we could get rid of this entirely.
https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:713: BuildHardReturn(); On 2017/02/09 13:33:49, rmcilroy wrote: > On 2017/02/09 13:23:36, Dan Ehrenberg wrote: > > On 2017/02/09 12:53:07, caitp wrote: > > > On 2017/02/09 06:40:00, Dan Ehrenberg wrote: > > > > Why is this a hard return? I would expect implicit returns to resolve the > > > > promise with 'undefined', rather than actually return 'undefined'. > > > > > > This change is just too shrink bytecode a bit. > > > > > > The parser inserts an actual "implicit return" as the last statement in the > > try > > > block, so this thing is actually unreachable. Even so, all blocks need a > > return, > > > so this generates "LdaUndefined Return" instead of the full promise > resolution > > > thing, because for async functions and generators it doesn't actually > matter. > > > > I thought this would be reached in a case like "async function() { }". How > does > > that end up resolving the Promise with undefined? > > I didn't realize this was only a to optimize bytecode size, I thought it was > necessary with the promise resolution. I'm a bit less happy with having this as > a hard return just because it's dead code in async code. Could we just make it a > normal return for now? > > Going forward, if the parser is smart enough to add implicit returns for async > functions, could it add implicit returns for all functions? If so we could get > rid of this entirely. My understanding is that this is needed to ensure that the last block has an exit, in order to be a valid graph, which is presumably important for TF. So, ordinary functions could have an inserted `return undefined` in the parser, but the return that the parser inserts for async functions doesn't end up being the exit for the last block in the function, due to the implicit try/catch. So in that situation, the implicit return here would still be needed.
On 2017/02/09 13:23:22, Dan Ehrenberg wrote: > On 2017/02/09 12:49:42, caitp wrote: > > On 2017/02/09 06:46:58, Dan Ehrenberg wrote: > > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h > > > File src/ast/ast.h (right): > > > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h#newcode899 > > > src/ast/ast.h:899: enum Type { kNormal, kHardReturn }; > > > On 2017/02/08 23:22:15, caitp wrote: > > > > On 2017/02/08 21:12:56, Dan Ehrenberg wrote: > > > > > Bikeshedding suggestion: Instead, have normal returns and async returns. > > > > > RewriteReturn would turn return statements into async return statements > > when > > > > in > > > > > an async function. RewriteReturn only runs for syntactic return > > statements, > > > > not > > > > > for ones that come out of desugaring, so this should end up doing the > same > > > > > thing, right? > > > > > > > > As explained on IRC: > > > > > > > > The key fix for the try/finally issue is to remove the > > Parser::RewriteReturn() > > > > stuff for async functions. > > > > > > > > In place of calling `ResolvePromise(<function_promise>, return_value);` in > > the > > > > Parser/AST, it's done in BCG::BuildReturn(). > > > > > > > > But, because this version doesn't move the implicit control flow > (top-level > > > > try/catch blocks) into BCG, and the parser still generates the `return > > > > RejectPromise(). <function_promise>;`, or effectively `return > > > > <function_promise>;` at the end of those implicit try/catch blocks. As > noted > > > > above, BCG would turn this into `ResolvePromise(<function_promise>, > > > > <function_promise>)`, which is a circular promise resolution. > > > > > > > > This "hard return" bit tells the BCG not to transform the return value > into > > a > > > > ResolvePromise() call, so it can avoid the circular promise resolution. > > > > > > > > Please let me know if I'm not explaining this clearly. > > > > > > I see how you need to differentiate between returns that need the promise > > > behavior and returns that don't in order to avoid introducing a bug, and the > > > logic seems valid here. Above, I was making a style suggestion about > phrasing > > > that logic--I though the logic I described would end up generating the same > > > bytecode. > > > > > > I am still missing something about how this fixes the issue. You end up > > > desugaring (in the BCG now) to the same thing in some cases. Which cases > > differ? > > > I can't find them. Is this all about the implicit 'return undefined' > generated > > > at the end? > > > > It only changes the way ReturnStatements in authored code work, by making them > > work with the normal delegates command system for try/finally. The returns > > inserted by the parser don't change, and use the hard-return mechanic to > prevent > > them from changing. > > > > So, yeah, it only affects actual return statements written in the source text. > > "by making them work with the normal delegates command system for try/finally." > > OK, I guess this is the part I don't understand. What's this interaction here, > and why wasn't it working before? So, lets walk through this... Suppose an author writes the following: ``` try { return foo; } finally { doThingsOnCleanup(); } Without this patch: The parser would return that return statement into `return ResolvePromise(P, foo), P;`. When evaluating the return statement, we end up with P (the function promise) in the accumulator. At this point, `P` is already resolved with `foo`, because that happened during `VisitForAccumulatorValue(stmt->expression());` in BCG::VisitReturnStatement(); Now, `P` still falls through the ControlScope deferred command system, so eventually, we end up at a BuildReturn(), with the accumulator loaded with the deferred value P, followed by a Return(); This is why if there was an Await in the try/finally block, the main promise was resolved before the finally blocks were finished evaluated (because, Await() yields P in async functions). Similarly, if there was another return statement in the finally block, P was already resolved, so the second ResolvePromise didn't do anything --- and similarly, if there was a throw, that wouldn't do anything either. Now, with this patch: The parser no longer transforms return statements into `return ResolvePromise(P, value), P;`, instead, BCG does that in BuildReturn. This means that the ResolvePromise step is deferred until all finally blocks have been properly finished, including Awaits which would yield control to the outside. BUT, this patch does not touch the implicit control flow. So, the parser is still inserting some `return ResolvePromise(P, value), P;` and `return RejectPromise(P, exception), P;` statements via BuildResolvePromise and BuildRejectPromise(). Because BCG now rewrites the return value into ResolvePromise(), this adds a concept of a HardReturn AST node (which is just a ReturnStatement node with a flag set). This bit tells BCG not to rewrite the return value, essentially, so that the parser-inserted `return P` doesn't become `return ResolvePromise(P, P)`, which is of course an error. --- Now, at the end of all of this, BCG still adds an implicit return if needed, I believe this is to ensure that the graph is still valid (each block has single an entry and exit). Before this patch, BCG would emit `LdaUndefined; Return;` for this. But with this patch, because BuildReturn resolves a promise, it emits a fair bit more code. I felt that it would be better to keep the graph more or less how it used to be, so I changed the BuildReturn to a BuildHardReturn(); to prevent any transformation to the return value which would cause the bytecode size to grow. So, with the BuildHardReturn() call, it is once again `LdaUndefined; Return;`, as it was previously. For async functions, this block is actually unreachable, there's no reachable successor to the top-level try/catch blocks. Same for generators, there's no reachable successor to the try/finally block in generators. Nevertheless, we still emit these 2 bytes at the end of them to ensure the graph is valid. I don't mind changing the BuildHardReturn() there into a BuildReturn() again, but it will generate more bytes than were previously generated for async functions, and they're all unreachable. If we're sure we want to do that, I'll make that change --- otherwise, if people see this as too complicated or confusing, maybe we should just land Sathya's version and be done with it.
Description was changed from ========== [async-await] (simpler) fix for Return in try/finally in async functions Alternative approach to https://codereview.chromium.org/2667983004/, which does not depend on implicit control flow changes from https://codereview.chromium.org/2664083002 The downside of not doing the implicit control flow in BCG is, need to distinguish between whether function promise is being returned by authored code, as in the following case: ``` var fn_promise; async function broken() { await Promise.resolve(); return fn_promise; } fn_promise = broken(); // `return fn_promise` is circularly resolved ``` from when the promise is returned from desugaring in the Parser, ``` async function broken(a = b, b) {} broken(); // try { <parameter_init_block> } // catch (e) { // return RejectPromise(<fn_promise>, e), <fn_promise>; // } ``` Unfortunately, we can't generate bytecode which tests if the returned value is the function promise, and skip resolving the Promise in that case, because that would break the valid case #1. BUG=v8:5896, v8:4483 R=littledan@chromium.org, neis@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, gsathya@chromium.org ========== to ========== [async-await] (simpler) fix for Return in try/finally in async functions Alternative approach to https://codereview.chromium.org/2667983004/, which does not depend on implicit control flow changes from https://codereview.chromium.org/2664083002 - Remove handling for `async function` from Parser::RewriteReturn(). This functionality is moved to BytecodeGenerator::BuildReturn(). This ensures that promise resolution is deferred until all finally blocks are evaluated fully. - Add a concept of "hard returns" to the AST, which allows parser-generated returns in Async Functions to not have their `return <Promise>;` step from being rewritten as `ResolvePromise(<Promise>, <Promise>); return <Promise>;` by the change mentioned above. This preserves the existing behaviour for return statements inserted by desugaring in the parser. - A slight change to the implicit Return in BytecodeGenerator::GenerateBytecode(): Previously, this would always emit `LdaUndefined; Return;`, but with the first change mentioned in this description, this becomes a fair bit larger for async functions. Because this code is unreachable, I've modified it slightly so that it continues to generate `LdaUndefined; Return;` for async functions. BUG=v8:5896, v8:4483 R=littledan@chromium.org, neis@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, gsathya@chromium.org ==========
On 2017/02/09 13:38:47, caitp wrote: > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > File src/interpreter/bytecode-generator.cc (right): > > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > src/interpreter/bytecode-generator.cc:713: BuildHardReturn(); > On 2017/02/09 13:33:49, rmcilroy wrote: > > On 2017/02/09 13:23:36, Dan Ehrenberg wrote: > > > On 2017/02/09 12:53:07, caitp wrote: > > > > On 2017/02/09 06:40:00, Dan Ehrenberg wrote: > > > > > Why is this a hard return? I would expect implicit returns to resolve > the > > > > > promise with 'undefined', rather than actually return 'undefined'. > > > > > > > > This change is just too shrink bytecode a bit. > > > > > > > > The parser inserts an actual "implicit return" as the last statement in > the > > > try > > > > block, so this thing is actually unreachable. Even so, all blocks need a > > > return, > > > > so this generates "LdaUndefined Return" instead of the full promise > > resolution > > > > thing, because for async functions and generators it doesn't actually > > matter. > > > > > > I thought this would be reached in a case like "async function() { }". How > > does > > > that end up resolving the Promise with undefined? > > > > I didn't realize this was only a to optimize bytecode size, I thought it was > > necessary with the promise resolution. I'm a bit less happy with having this > as > > a hard return just because it's dead code in async code. Could we just make it > a > > normal return for now? > > > > Going forward, if the parser is smart enough to add implicit returns for async > > functions, could it add implicit returns for all functions? If so we could get > > rid of this entirely. > > My understanding is that this is needed to ensure that the last block has an > exit, in order to be a valid graph, which is presumably important for TF. This is nothing to do with TF. It's required to have valid code, otherwise functions such as "function() { }" will have no return bytecode at the end of the function and would fall of the end of the bytecode array without returning since the parser doesn't insert the implicit return. > So, ordinary functions could have an inserted `return undefined` in the parser, > but the return that the parser inserts for async functions doesn't end up being > the exit for the last block in the function, due to the implicit try/catch. So > in that situation, the implicit return here would still be needed. Nope, if it's dead code and the parser inserts implict returns everywhere they are needed, then this code wouldn't be needed.
On 2017/02/09 14:47:38, rmcilroy wrote: > On 2017/02/09 13:38:47, caitp wrote: > > > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > > File src/interpreter/bytecode-generator.cc (right): > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > > src/interpreter/bytecode-generator.cc:713: BuildHardReturn(); > > On 2017/02/09 13:33:49, rmcilroy wrote: > > > On 2017/02/09 13:23:36, Dan Ehrenberg wrote: > > > > On 2017/02/09 12:53:07, caitp wrote: > > > > > On 2017/02/09 06:40:00, Dan Ehrenberg wrote: > > > > > > Why is this a hard return? I would expect implicit returns to resolve > > the > > > > > > promise with 'undefined', rather than actually return 'undefined'. > > > > > > > > > > This change is just too shrink bytecode a bit. > > > > > > > > > > The parser inserts an actual "implicit return" as the last statement in > > the > > > > try > > > > > block, so this thing is actually unreachable. Even so, all blocks need a > > > > return, > > > > > so this generates "LdaUndefined Return" instead of the full promise > > > resolution > > > > > thing, because for async functions and generators it doesn't actually > > > matter. > > > > > > > > I thought this would be reached in a case like "async function() { }". How > > > does > > > > that end up resolving the Promise with undefined? > > > > > > I didn't realize this was only a to optimize bytecode size, I thought it was > > > necessary with the promise resolution. I'm a bit less happy with having this > > as > > > a hard return just because it's dead code in async code. Could we just make > it > > a > > > normal return for now? > > > > > > Going forward, if the parser is smart enough to add implicit returns for > async > > > functions, could it add implicit returns for all functions? If so we could > get > > > rid of this entirely. > > > > My understanding is that this is needed to ensure that the last block has an > > exit, in order to be a valid graph, which is presumably important for TF. > > This is nothing to do with TF. It's required to have valid code, otherwise > functions such as "function() { }" will have no return bytecode at the end of > the function and would fall of the end of the bytecode array without returning > since the parser doesn't insert the implicit return. > > > So, ordinary functions could have an inserted `return undefined` in the > parser, > > but the return that the parser inserts for async functions doesn't end up > being > > the exit for the last block in the function, due to the implicit try/catch. So > > in that situation, the implicit return here would still be needed. > > Nope, if it's dead code and the parser inserts implict returns everywhere they > are needed, then this code wouldn't be needed. Okay, well there's an fails in BytecodeArrayBuilder that fires without it.
On 2017/02/09 14:50:44, caitp wrote: > On 2017/02/09 14:47:38, rmcilroy wrote: > > On 2017/02/09 13:38:47, caitp wrote: > > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > > > File src/interpreter/bytecode-generator.cc (right): > > > > > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > > > src/interpreter/bytecode-generator.cc:713: BuildHardReturn(); > > > On 2017/02/09 13:33:49, rmcilroy wrote: > > > > On 2017/02/09 13:23:36, Dan Ehrenberg wrote: > > > > > On 2017/02/09 12:53:07, caitp wrote: > > > > > > On 2017/02/09 06:40:00, Dan Ehrenberg wrote: > > > > > > > Why is this a hard return? I would expect implicit returns to > resolve > > > the > > > > > > > promise with 'undefined', rather than actually return 'undefined'. > > > > > > > > > > > > This change is just too shrink bytecode a bit. > > > > > > > > > > > > The parser inserts an actual "implicit return" as the last statement > in > > > the > > > > > try > > > > > > block, so this thing is actually unreachable. Even so, all blocks need > a > > > > > return, > > > > > > so this generates "LdaUndefined Return" instead of the full promise > > > > resolution > > > > > > thing, because for async functions and generators it doesn't actually > > > > matter. > > > > > > > > > > I thought this would be reached in a case like "async function() { }". > How > > > > does > > > > > that end up resolving the Promise with undefined? > > > > > > > > I didn't realize this was only a to optimize bytecode size, I thought it > was > > > > necessary with the promise resolution. I'm a bit less happy with having > this > > > as > > > > a hard return just because it's dead code in async code. Could we just > make > > it > > > a > > > > normal return for now? > > > > > > > > Going forward, if the parser is smart enough to add implicit returns for > > async > > > > functions, could it add implicit returns for all functions? If so we could > > get > > > > rid of this entirely. > > > > > > My understanding is that this is needed to ensure that the last block has an > > > exit, in order to be a valid graph, which is presumably important for TF. > > > > This is nothing to do with TF. It's required to have valid code, otherwise > > functions such as "function() { }" will have no return bytecode at the end of > > the function and would fall of the end of the bytecode array without returning > > since the parser doesn't insert the implicit return. > > > > > So, ordinary functions could have an inserted `return undefined` in the > > parser, > > > but the return that the parser inserts for async functions doesn't end up > > being > > > the exit for the last block in the function, due to the implicit try/catch. > So > > > in that situation, the implicit return here would still be needed. > > > > Nope, if it's dead code and the parser inserts implict returns everywhere they > > are needed, then this code wouldn't be needed. > > Okay, well there's an fails in BytecodeArrayBuilder that fires without it. sorry, laptop failed to type "an assertion that fails in BytecodeArrayBuilder that fires without it."
On 2017/02/09 14:51:15, caitp wrote: > On 2017/02/09 14:50:44, caitp wrote: > > On 2017/02/09 14:47:38, rmcilroy wrote: > > > On 2017/02/09 13:38:47, caitp wrote: > > > > > > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > > > > File src/interpreter/bytecode-generator.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > > > > src/interpreter/bytecode-generator.cc:713: BuildHardReturn(); > > > > On 2017/02/09 13:33:49, rmcilroy wrote: > > > > > On 2017/02/09 13:23:36, Dan Ehrenberg wrote: > > > > > > On 2017/02/09 12:53:07, caitp wrote: > > > > > > > On 2017/02/09 06:40:00, Dan Ehrenberg wrote: > > > > > > > > Why is this a hard return? I would expect implicit returns to > > resolve > > > > the > > > > > > > > promise with 'undefined', rather than actually return 'undefined'. > > > > > > > > > > > > > > This change is just too shrink bytecode a bit. > > > > > > > > > > > > > > The parser inserts an actual "implicit return" as the last statement > > in > > > > the > > > > > > try > > > > > > > block, so this thing is actually unreachable. Even so, all blocks > need > > a > > > > > > return, > > > > > > > so this generates "LdaUndefined Return" instead of the full promise > > > > > resolution > > > > > > > thing, because for async functions and generators it doesn't > actually > > > > > matter. > > > > > > > > > > > > I thought this would be reached in a case like "async function() { }". > > How > > > > > does > > > > > > that end up resolving the Promise with undefined? > > > > > > > > > > I didn't realize this was only a to optimize bytecode size, I thought it > > was > > > > > necessary with the promise resolution. I'm a bit less happy with having > > this > > > > as > > > > > a hard return just because it's dead code in async code. Could we just > > make > > > it > > > > a > > > > > normal return for now? > > > > > > > > > > Going forward, if the parser is smart enough to add implicit returns for > > > async > > > > > functions, could it add implicit returns for all functions? If so we > could > > > get > > > > > rid of this entirely. > > > > > > > > My understanding is that this is needed to ensure that the last block has > an > > > > exit, in order to be a valid graph, which is presumably important for TF. > > > > > > This is nothing to do with TF. It's required to have valid code, otherwise > > > functions such as "function() { }" will have no return bytecode at the end > of > > > the function and would fall of the end of the bytecode array without > returning > > > since the parser doesn't insert the implicit return. > > > > > > > So, ordinary functions could have an inserted `return undefined` in the > > > parser, > > > > but the return that the parser inserts for async functions doesn't end up > > > being > > > > the exit for the last block in the function, due to the implicit > try/catch. > > So > > > > in that situation, the implicit return here would still be needed. > > > > > > Nope, if it's dead code and the parser inserts implict returns everywhere > they > > > are needed, then this code wouldn't be needed. > > > > Okay, well there's an fails in BytecodeArrayBuilder that fires without it. > > sorry, laptop failed to type "an assertion that fails in BytecodeArrayBuilder > that fires without it." That's just checking that we didn't miss an implicit return in the final basic block. We could probably change it to check that there was a return in the last basic block which actually had some bytecode that would actually be run, although to do that we would need to figure out which blocks were actually dead code (and at that point we could just strip them out at which point there would always be a return in the final block). I'm presuming bytecode-dead-code-optimizer doesn't manage to strip out the final dead block in async functions?
On 2017/02/09 14:58:12, rmcilroy wrote: > On 2017/02/09 14:51:15, caitp wrote: > > On 2017/02/09 14:50:44, caitp wrote: > > > On 2017/02/09 14:47:38, rmcilroy wrote: > > > > On 2017/02/09 13:38:47, caitp wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > > > > > File src/interpreter/bytecode-generator.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > > > > > src/interpreter/bytecode-generator.cc:713: BuildHardReturn(); > > > > > On 2017/02/09 13:33:49, rmcilroy wrote: > > > > > > On 2017/02/09 13:23:36, Dan Ehrenberg wrote: > > > > > > > On 2017/02/09 12:53:07, caitp wrote: > > > > > > > > On 2017/02/09 06:40:00, Dan Ehrenberg wrote: > > > > > > > > > Why is this a hard return? I would expect implicit returns to > > > resolve > > > > > the > > > > > > > > > promise with 'undefined', rather than actually return > 'undefined'. > > > > > > > > > > > > > > > > This change is just too shrink bytecode a bit. > > > > > > > > > > > > > > > > The parser inserts an actual "implicit return" as the last > statement > > > in > > > > > the > > > > > > > try > > > > > > > > block, so this thing is actually unreachable. Even so, all blocks > > need > > > a > > > > > > > return, > > > > > > > > so this generates "LdaUndefined Return" instead of the full > promise > > > > > > resolution > > > > > > > > thing, because for async functions and generators it doesn't > > actually > > > > > > matter. > > > > > > > > > > > > > > I thought this would be reached in a case like "async function() { > }". > > > How > > > > > > does > > > > > > > that end up resolving the Promise with undefined? > > > > > > > > > > > > I didn't realize this was only a to optimize bytecode size, I thought > it > > > was > > > > > > necessary with the promise resolution. I'm a bit less happy with > having > > > this > > > > > as > > > > > > a hard return just because it's dead code in async code. Could we just > > > make > > > > it > > > > > a > > > > > > normal return for now? > > > > > > > > > > > > Going forward, if the parser is smart enough to add implicit returns > for > > > > async > > > > > > functions, could it add implicit returns for all functions? If so we > > could > > > > get > > > > > > rid of this entirely. > > > > > > > > > > My understanding is that this is needed to ensure that the last block > has > > an > > > > > exit, in order to be a valid graph, which is presumably important for > TF. > > > > > > > > This is nothing to do with TF. It's required to have valid code, otherwise > > > > functions such as "function() { }" will have no return bytecode at the end > > of > > > > the function and would fall of the end of the bytecode array without > > returning > > > > since the parser doesn't insert the implicit return. > > > > > > > > > So, ordinary functions could have an inserted `return undefined` in the > > > > parser, > > > > > but the return that the parser inserts for async functions doesn't end > up > > > > being > > > > > the exit for the last block in the function, due to the implicit > > try/catch. > > > So > > > > > in that situation, the implicit return here would still be needed. > > > > > > > > Nope, if it's dead code and the parser inserts implict returns everywhere > > they > > > > are needed, then this code wouldn't be needed. > > > > > > Okay, well there's an fails in BytecodeArrayBuilder that fires without it. > > > > sorry, laptop failed to type "an assertion that fails in BytecodeArrayBuilder > > that fires without it." > > That's just checking that we didn't miss an implicit return in the final basic > block. We could probably change it to check that there was a return in the last > basic block which actually had some bytecode that would actually be run, > although to do that we would need to figure out which blocks were actually dead > code (and at that point we could just strip them out at which point there would > always be a return in the final block). I'm presuming > bytecode-dead-code-optimizer doesn't manage to strip out the final dead block in > async functions? Based on what test-bytecode-generator emits, it seems like it doesn't (unless that doesn't include the dead-code-optimizer step)
On 2017/02/09 14:59:37, caitp wrote: > On 2017/02/09 14:58:12, rmcilroy wrote: > > On 2017/02/09 14:51:15, caitp wrote: > > > On 2017/02/09 14:50:44, caitp wrote: > > > > On 2017/02/09 14:47:38, rmcilroy wrote: > > > > > On 2017/02/09 13:38:47, caitp wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > > > > > > File src/interpreter/bytecode-generator.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/interpreter/bytecod... > > > > > > src/interpreter/bytecode-generator.cc:713: BuildHardReturn(); > > > > > > On 2017/02/09 13:33:49, rmcilroy wrote: > > > > > > > On 2017/02/09 13:23:36, Dan Ehrenberg wrote: > > > > > > > > On 2017/02/09 12:53:07, caitp wrote: > > > > > > > > > On 2017/02/09 06:40:00, Dan Ehrenberg wrote: > > > > > > > > > > Why is this a hard return? I would expect implicit returns to > > > > resolve > > > > > > the > > > > > > > > > > promise with 'undefined', rather than actually return > > 'undefined'. > > > > > > > > > > > > > > > > > > This change is just too shrink bytecode a bit. > > > > > > > > > > > > > > > > > > The parser inserts an actual "implicit return" as the last > > statement > > > > in > > > > > > the > > > > > > > > try > > > > > > > > > block, so this thing is actually unreachable. Even so, all > blocks > > > need > > > > a > > > > > > > > return, > > > > > > > > > so this generates "LdaUndefined Return" instead of the full > > promise > > > > > > > resolution > > > > > > > > > thing, because for async functions and generators it doesn't > > > actually > > > > > > > matter. > > > > > > > > > > > > > > > > I thought this would be reached in a case like "async function() { > > }". > > > > How > > > > > > > does > > > > > > > > that end up resolving the Promise with undefined? > > > > > > > > > > > > > > I didn't realize this was only a to optimize bytecode size, I > thought > > it > > > > was > > > > > > > necessary with the promise resolution. I'm a bit less happy with > > having > > > > this > > > > > > as > > > > > > > a hard return just because it's dead code in async code. Could we > just > > > > make > > > > > it > > > > > > a > > > > > > > normal return for now? > > > > > > > > > > > > > > Going forward, if the parser is smart enough to add implicit returns > > for > > > > > async > > > > > > > functions, could it add implicit returns for all functions? If so we > > > could > > > > > get > > > > > > > rid of this entirely. > > > > > > > > > > > > My understanding is that this is needed to ensure that the last block > > has > > > an > > > > > > exit, in order to be a valid graph, which is presumably important for > > TF. > > > > > > > > > > This is nothing to do with TF. It's required to have valid code, > otherwise > > > > > functions such as "function() { }" will have no return bytecode at the > end > > > of > > > > > the function and would fall of the end of the bytecode array without > > > returning > > > > > since the parser doesn't insert the implicit return. > > > > > > > > > > > So, ordinary functions could have an inserted `return undefined` in > the > > > > > parser, > > > > > > but the return that the parser inserts for async functions doesn't end > > up > > > > > being > > > > > > the exit for the last block in the function, due to the implicit > > > try/catch. > > > > So > > > > > > in that situation, the implicit return here would still be needed. > > > > > > > > > > Nope, if it's dead code and the parser inserts implict returns > everywhere > > > they > > > > > are needed, then this code wouldn't be needed. > > > > > > > > Okay, well there's an fails in BytecodeArrayBuilder that fires without it. > > > > > > sorry, laptop failed to type "an assertion that fails in > BytecodeArrayBuilder > > > that fires without it." > > > > That's just checking that we didn't miss an implicit return in the final basic > > block. We could probably change it to check that there was a return in the > last > > basic block which actually had some bytecode that would actually be run, > > although to do that we would need to figure out which blocks were actually > dead > > code (and at that point we could just strip them out at which point there > would > > always be a return in the final block). I'm presuming > > bytecode-dead-code-optimizer doesn't manage to strip out the final dead block > in > > async functions? > > Based on what test-bytecode-generator emits, it seems like it doesn't (unless > that doesn't include the dead-code-optimizer step) Yeah it should include dead-code-optimizer. The optimizer isn't very smart though - if it sees a BytecodeLabel being bound it assumes something will jump to that label, even if the jump point is also dead - that's probably what's happening here.
On 2017/02/09 14:06:39, caitp wrote: > On 2017/02/09 13:23:22, Dan Ehrenberg wrote: > > On 2017/02/09 12:49:42, caitp wrote: > > > On 2017/02/09 06:46:58, Dan Ehrenberg wrote: > > > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h > > > > File src/ast/ast.h (right): > > > > > > > > > > https://codereview.chromium.org/2685683002/diff/20001/src/ast/ast.h#newcode899 > > > > src/ast/ast.h:899: enum Type { kNormal, kHardReturn }; > > > > On 2017/02/08 23:22:15, caitp wrote: > > > > > On 2017/02/08 21:12:56, Dan Ehrenberg wrote: > > > > > > Bikeshedding suggestion: Instead, have normal returns and async > returns. > > > > > > RewriteReturn would turn return statements into async return > statements > > > when > > > > > in > > > > > > an async function. RewriteReturn only runs for syntactic return > > > statements, > > > > > not > > > > > > for ones that come out of desugaring, so this should end up doing the > > same > > > > > > thing, right? > > > > > > > > > > As explained on IRC: > > > > > > > > > > The key fix for the try/finally issue is to remove the > > > Parser::RewriteReturn() > > > > > stuff for async functions. > > > > > > > > > > In place of calling `ResolvePromise(<function_promise>, return_value);` > in > > > the > > > > > Parser/AST, it's done in BCG::BuildReturn(). > > > > > > > > > > But, because this version doesn't move the implicit control flow > > (top-level > > > > > try/catch blocks) into BCG, and the parser still generates the `return > > > > > RejectPromise(). <function_promise>;`, or effectively `return > > > > > <function_promise>;` at the end of those implicit try/catch blocks. As > > noted > > > > > above, BCG would turn this into `ResolvePromise(<function_promise>, > > > > > <function_promise>)`, which is a circular promise resolution. > > > > > > > > > > This "hard return" bit tells the BCG not to transform the return value > > into > > > a > > > > > ResolvePromise() call, so it can avoid the circular promise resolution. > > > > > > > > > > Please let me know if I'm not explaining this clearly. > > > > > > > > I see how you need to differentiate between returns that need the promise > > > > behavior and returns that don't in order to avoid introducing a bug, and > the > > > > logic seems valid here. Above, I was making a style suggestion about > > phrasing > > > > that logic--I though the logic I described would end up generating the > same > > > > bytecode. > > > > > > > > I am still missing something about how this fixes the issue. You end up > > > > desugaring (in the BCG now) to the same thing in some cases. Which cases > > > differ? > > > > I can't find them. Is this all about the implicit 'return undefined' > > generated > > > > at the end? > > > > > > It only changes the way ReturnStatements in authored code work, by making > them > > > work with the normal delegates command system for try/finally. The returns > > > inserted by the parser don't change, and use the hard-return mechanic to > > prevent > > > them from changing. > > > > > > So, yeah, it only affects actual return statements written in the source > text. > > > > "by making them work with the normal delegates command system for > try/finally." > > > > OK, I guess this is the part I don't understand. What's this interaction here, > > and why wasn't it working before? > > So, lets walk through this... > > Suppose an author writes the following: > > ``` > try { > return foo; > } finally { > doThingsOnCleanup(); > } > > Without this patch: > > The parser would return that return statement into `return ResolvePromise(P, > foo), P;`. > > When evaluating the return statement, we end up with P (the function promise) in > the accumulator. At this point, `P` is already resolved with `foo`, because that > happened during `VisitForAccumulatorValue(stmt->expression());` in > BCG::VisitReturnStatement(); > > Now, `P` still falls through the ControlScope deferred command system, so > eventually, we end up at a BuildReturn(), with the accumulator loaded with the > deferred value P, followed by a Return(); > > This is why if there was an Await in the try/finally block, the main promise was > resolved before the finally blocks were finished evaluated (because, Await() > yields P in async functions). Similarly, if there was another return statement > in the finally block, P was already resolved, so the second ResolvePromise > didn't do anything --- and similarly, if there was a throw, that wouldn't do > anything either. > > Now, with this patch: > > The parser no longer transforms return statements into `return ResolvePromise(P, > value), P;`, instead, BCG does that in BuildReturn. This means that the > ResolvePromise step is deferred until all finally blocks have been properly > finished, including Awaits which would yield control to the outside. > > BUT, this patch does not touch the implicit control flow. So, the parser is > still inserting some `return ResolvePromise(P, value), P;` and `return > RejectPromise(P, exception), P;` statements via BuildResolvePromise and > BuildRejectPromise(). > > Because BCG now rewrites the return value into ResolvePromise(), this adds a > concept of a HardReturn AST node (which is just a ReturnStatement node with a > flag set). This bit tells BCG not to rewrite the return value, essentially, so > that the parser-inserted `return P` doesn't become `return ResolvePromise(P, > P)`, which is of course an error. > > --- > > Now, at the end of all of this, BCG still adds an implicit return if needed, I > believe this is to ensure that the graph is still valid (each block has single > an entry and exit). > > Before this patch, BCG would emit `LdaUndefined; Return;` for this. But with > this patch, because BuildReturn resolves a promise, it emits a fair bit more > code. I felt that it would be better to keep the graph more or less how it used > to be, so I changed the BuildReturn to a BuildHardReturn(); to prevent any > transformation to the return value which would cause the bytecode size to grow. > > So, with the BuildHardReturn() call, it is once again `LdaUndefined; Return;`, > as it was previously. For async functions, this block is actually unreachable, > there's no reachable successor to the top-level try/catch blocks. Same for > generators, there's no reachable successor to the try/finally block in > generators. Nevertheless, we still emit these 2 bytes at the end of them to > ensure the graph is valid. > > I don't mind changing the BuildHardReturn() there into a BuildReturn() again, > but it will generate more bytes than were previously generated for async > functions, and they're all unreachable. > > If we're sure we want to do that, I'll make that change --- otherwise, if people > see this as too complicated or confusing, maybe we should just land Sathya's > version and be done with it. OK, I get it now, what I was missing was that BuildReturn is not called directly by VisitReturnStatement, but instead, it's called by the ControlScope system *after* emitting code to visit all of the finally blocks (which you can see in BytecodeGenerator::ControlScopeForTryFinally::Execute). So, if a finally block has a return statement in it, we get the right semantics to resolve the promise with that one; otherwise, if we continue to the original return statement and resolve the Promise here. It would be a little cleaner in my mind if we had an AsyncReturn statement, rather than differentiating on the other end Return/HardReturn, but given the logic for CMDs in the bytecode generator, it seems like it would be a bit more code to make a CMD_ASYNC_RETURN; HardReturn gets around this because the parser only emits it in places where you can skip the whole ControlScope logic. The logic's all a bit subtle and could use a few more comments (or making an AsyncReturn AST node instead with all that other logic; either way). All put together, it's really nice how this version takes advantage of the existing infrastructure in ControlScope and puts the bookkeeping at compile-time, rather than at runtime with parallel infrastructure as in the alternative patch https://codereview.chromium.org/2672313003/ . In some way it feels lucky that the interpreter is structured this way, but in another way, I guess it is because it is solving the same problems (figuring out what to return when there are try/finally blocks) as we are solving here, so it makes sense that there's common logic.
So given that we're on the same page with that (reusing the ControlScope machinery rather than duplicating it in the AST desugaring...), Ross, do you have a preference here? I guess the things that are unclear to me are: 1) For the implicit return: - do a normal BuildReturn() for the implicit return at the end of GenerateBytecode(), and fix the dead code optimization so that this doesn't produce unnecessarily large code for async functions, OR - keep doing the hard return for the implicit return, so that the dead code at the end of async functions is at least the same as it was before, and relatively small and 2) For the overall approach: - implement Dan's suggestion (new "AsyncReturn" AST node that generates the ResolvePromise thing, with a new deferred command which is virtually identical to CMD_RETURN, but builds a promise resolution instead) OR - keep the approach that exists here, and remove the special "hard return" thing once the outer try/catch is not added through the AST (giving BCG more control over what is actually returned) I'm happy with any combination of any of these ideas, depending on what people would prefer to have upstream.
On 2017/02/09 16:24:42, caitp wrote: > So given that we're on the same page with that (reusing the ControlScope > machinery rather than duplicating it in the AST desugaring...), Ross, do you > have a preference here? > > I guess the things that are unclear to me are: > > 1) For the implicit return: > - do a normal BuildReturn() for the implicit return at the end of > GenerateBytecode(), and fix the dead code optimization so that this doesn't > produce unnecessarily large code for async functions, OR > - keep doing the hard return for the implicit return, so that the dead code at > the end of async functions is at least the same as it was before, and relatively > small > > and > > 2) For the overall approach: > - implement Dan's suggestion (new "AsyncReturn" AST node that generates the > ResolvePromise thing, with a new deferred command which is virtually identical > to CMD_RETURN, but builds a promise resolution instead) OR > - keep the approach that exists here, and remove the special "hard return" > thing once the outer try/catch is not added through the AST (giving BCG more > control over what is actually returned) > > I'm happy with any combination of any of these ideas, depending on what people > would prefer to have upstream. Personally I like Dan's idea of a AsyncReturn node with the new CMD_ASYNC_RETURN. If we do that then 1) shouldnt be a problem since the implicit return won't add any of the extra async logic anyway.
On 2017/02/09 16:24:42, caitp wrote: > So given that we're on the same page with that (reusing the ControlScope > machinery rather than duplicating it in the AST desugaring...), Ross, do you > have a preference here? > > I guess the things that are unclear to me are: > > 1) For the implicit return: > - do a normal BuildReturn() for the implicit return at the end of > GenerateBytecode(), and fix the dead code optimization so that this doesn't > produce unnecessarily large code for async functions, OR > - keep doing the hard return for the implicit return, so that the dead code at > the end of async functions is at least the same as it was before, and relatively > small > > and > > 2) For the overall approach: > - implement Dan's suggestion (new "AsyncReturn" AST node that generates the > ResolvePromise thing, with a new deferred command which is virtually identical > to CMD_RETURN, but builds a promise resolution instead) OR > - keep the approach that exists here, and remove the special "hard return" > thing once the outer try/catch is not added through the AST (giving BCG more > control over what is actually returned) > > I'm happy with any combination of any of these ideas, depending on what people > would prefer to have upstream. Personally I like Dan's idea of a AsyncReturn node with the new CMD_ASYNC_RETURN. If we do that then 1) shouldnt be a problem since the implicit return won't add any of the extra async logic anyway.
The CQ bit was checked by caitp@igalia.com 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 caitp@igalia.com 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...
Description was changed from ========== [async-await] (simpler) fix for Return in try/finally in async functions Alternative approach to https://codereview.chromium.org/2667983004/, which does not depend on implicit control flow changes from https://codereview.chromium.org/2664083002 - Remove handling for `async function` from Parser::RewriteReturn(). This functionality is moved to BytecodeGenerator::BuildReturn(). This ensures that promise resolution is deferred until all finally blocks are evaluated fully. - Add a concept of "hard returns" to the AST, which allows parser-generated returns in Async Functions to not have their `return <Promise>;` step from being rewritten as `ResolvePromise(<Promise>, <Promise>); return <Promise>;` by the change mentioned above. This preserves the existing behaviour for return statements inserted by desugaring in the parser. - A slight change to the implicit Return in BytecodeGenerator::GenerateBytecode(): Previously, this would always emit `LdaUndefined; Return;`, but with the first change mentioned in this description, this becomes a fair bit larger for async functions. Because this code is unreachable, I've modified it slightly so that it continues to generate `LdaUndefined; Return;` for async functions. BUG=v8:5896, v8:4483 R=littledan@chromium.org, neis@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, gsathya@chromium.org ========== to ========== [async-await] (simpler) fix for Return in try/finally in async functions Alternative approach to https://codereview.chromium.org/2667983004/, which does not depend on implicit control flow changes from https://codereview.chromium.org/2664083002 - Remove handling for `async function` from Parser::RewriteReturn(). This functionality is moved to BytecodeGenerator::BuildAsyncReturn(). This ensures that promise resolution is deferred until all finally blocks are evaluated fully. - Add a new deferred command (CMD_ASYNC_RETURN), which instructs ControlScope to generate return code using BuildAsyncReturn rather than BuildReturn. - Parser has a new `NewReturnStatement()` helper which determines what type of return statement to generate based on the type of function. BUG=v8:5896, v8:4483 R=littledan@chromium.org, neis@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, gsathya@chromium.org ==========
On 2017/02/09 18:28:02, rmcilroy wrote: > On 2017/02/09 16:24:42, caitp wrote: > > So given that we're on the same page with that (reusing the ControlScope > > machinery rather than duplicating it in the AST desugaring...), Ross, do you > > have a preference here? > > > > I guess the things that are unclear to me are: > > > > 1) For the implicit return: > > - do a normal BuildReturn() for the implicit return at the end of > > GenerateBytecode(), and fix the dead code optimization so that this doesn't > > produce unnecessarily large code for async functions, OR > > - keep doing the hard return for the implicit return, so that the dead code > at > > the end of async functions is at least the same as it was before, and > relatively > > small > > > > and > > > > 2) For the overall approach: > > - implement Dan's suggestion (new "AsyncReturn" AST node that generates the > > ResolvePromise thing, with a new deferred command which is virtually identical > > to CMD_RETURN, but builds a promise resolution instead) OR > > - keep the approach that exists here, and remove the special "hard return" > > thing once the outer try/catch is not added through the AST (giving BCG more > > control over what is actually returned) > > > > I'm happy with any combination of any of these ideas, depending on what people > > would prefer to have upstream. > > Personally I like Dan's idea of a AsyncReturn node with the new > CMD_ASYNC_RETURN. If we do that then 1) shouldnt be a problem since the implicit > return won't add any of the extra async logic anyway. I've gone with that approach, it's only about ~30 more lines of code than the other way, so I guess that's not as bad as I thought.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Interpreter/ LGTM, thanks. https://codereview.chromium.org/2685683002/diff/60001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2685683002/diff/60001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1080: execution_control()->ReturnAccumulator(); nit - could you make this: if (stmt->is_async_return()) { execution_control()->AsyncReturnAccumulator(); } else { execution_control()->ReturnAccumulator(); }
Just need to hear some sign-off for changes to parsing/ast. https://codereview.chromium.org/2685683002/diff/60001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/2685683002/diff/60001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1080: execution_control()->ReturnAccumulator(); On 2017/02/09 23:31:45, rmcilroy wrote: > nit - could you make this: > if (stmt->is_async_return()) { > execution_control()->AsyncReturnAccumulator(); > } else { > execution_control()->ReturnAccumulator(); > } Done
littledan@chromium.org changed reviewers: + marja@chromium.org
lgtm I really like this version. Before submitting, could you get a review from gsathya on whether this seems like a good thing to do instead of his patch? I also added marja to review whether (architecturally) we want to be going in the direction of adding more things like this to the DeclarationScope. (idle thought: could this be a DeclarationScope subclass?) (Nit: Could you word-wrap the commit message?) https://codereview.chromium.org/2685683002/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2685683002/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:4276: zone()); Nit: Revert irrelevant formatting change https://codereview.chromium.org/2685683002/diff/80001/test/mjsunit/es8/async-... File test/mjsunit/es8/async-function-try-finally.js (right): https://codereview.chromium.org/2685683002/diff/80001/test/mjsunit/es8/async-... test/mjsunit/es8/async-function-try-finally.js:1: // Copyright 2017 the V8 project authors. All rights reserved. There's something about these semantics that was a little unexpected to me, which is that something like async function() { try { return new Promise() } finally { return; } } will return a Promise that resolves to undefined rather than hang forever (namely, that we don't wait on the first promise before returning the second one). This is what the spec prescribes, and it's what you implement here, but it was surprising to me, so I think it'd be nice if we had a test for it.
https://codereview.chromium.org/2685683002/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2685683002/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:4276: zone()); On 2017/02/10 07:51:31, Dan Ehrenberg wrote: > Nit: Revert irrelevant formatting change This isn't a formatting change, but the formatting does look weird, will fix that later https://codereview.chromium.org/2685683002/diff/80001/test/mjsunit/es8/async-... File test/mjsunit/es8/async-function-try-finally.js (right): https://codereview.chromium.org/2685683002/diff/80001/test/mjsunit/es8/async-... test/mjsunit/es8/async-function-try-finally.js:1: // Copyright 2017 the V8 project authors. All rights reserved. On 2017/02/10 07:51:31, Dan Ehrenberg wrote: > There's something about these semantics that was a little unexpected to me, > which is that something like > > async function() { > try { > return new Promise() > } finally { > return; > } > } > > will return a Promise that resolves to undefined rather than hang forever > (namely, that we don't wait on the first promise before returning the second > one). This is what the spec prescribes, and it's what you implement here, but it > was surprising to me, so I think it'd be nice if we had a test for it. Why would that return a promise resolved to undefined? Afaik the function promise shouldn't be resolved until the returned promise is, in that case. Is the returned promise actually resolved there?
https://codereview.chromium.org/2685683002/diff/80001/test/mjsunit/es8/async-... File test/mjsunit/es8/async-function-try-finally.js (right): https://codereview.chromium.org/2685683002/diff/80001/test/mjsunit/es8/async-... test/mjsunit/es8/async-function-try-finally.js:1: // Copyright 2017 the V8 project authors. All rights reserved. On 2017/02/10 08:05:07, caitp wrote: > On 2017/02/10 07:51:31, Dan Ehrenberg wrote: > > There's something about these semantics that was a little unexpected to me, > > which is that something like > > > > async function() { > > try { > > return new Promise() > > } finally { > > return; > > } > > } > > > > will return a Promise that resolves to undefined rather than hang forever > > (namely, that we don't wait on the first promise before returning the second > > one). This is what the spec prescribes, and it's what you implement here, but > it > > was surprising to me, so I think it'd be nice if we had a test for it. > > Why would that return a promise resolved to undefined? Afaik the function > promise shouldn't be resolved until the returned promise is, in that case. Is > the returned promise actually resolved there? Oh, i missed the finally block, gotcha. Yeah, will add a test for that
The CQ bit was checked by caitp@igalia.com 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...
Hmm, why doesn't this need changes in fullcodegen? https://codereview.chromium.org/2685683002/diff/80001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2685683002/diff/80001/src/ast/ast.h#newcode918 src/ast/ast.h:918: : public BitField<Type, JumpStatement::kNextBitFieldIndex, 2> {}; Nit: isn't 1 bit enough? https://codereview.chromium.org/2685683002/diff/80001/src/ast/scopes.h File src/ast/scopes.h (right): https://codereview.chromium.org/2685683002/diff/80001/src/ast/scopes.h#newcod... src/ast/scopes.h:897: Variable* promise_; Adding these increases the DeclarationScope size with a non-trivial amount... :/ Could you measure the size increase in some practical cases so that we can reason about it, whether it's significant or not? Since these aren't needed for all DeclarationScopes, would it be feasible to create subclasses where these members are? https://codereview.chromium.org/2685683002/diff/80001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2685683002/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:2482: temp->set_is_used(); Should the is_used be moved to DeclareGeneratorObjectVar - is there ever a case where we would DeclareGeneratorObjectVar but not mark it as used? https://codereview.chromium.org/2685683002/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:3098: promise->set_is_used(); Ditto
Description was changed from ========== [async-await] (simpler) fix for Return in try/finally in async functions Alternative approach to https://codereview.chromium.org/2667983004/, which does not depend on implicit control flow changes from https://codereview.chromium.org/2664083002 - Remove handling for `async function` from Parser::RewriteReturn(). This functionality is moved to BytecodeGenerator::BuildAsyncReturn(). This ensures that promise resolution is deferred until all finally blocks are evaluated fully. - Add a new deferred command (CMD_ASYNC_RETURN), which instructs ControlScope to generate return code using BuildAsyncReturn rather than BuildReturn. - Parser has a new `NewReturnStatement()` helper which determines what type of return statement to generate based on the type of function. BUG=v8:5896, v8:4483 R=littledan@chromium.org, neis@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, gsathya@chromium.org ========== to ========== [async-await] (simpler) fix for Return in try/finally in async functions Alternative approach to https://codereview.chromium.org/2667983004/, which does not depend on implicit control flow changes from https://codereview.chromium.org/2664083002 - Remove handling for `async function` from Parser::RewriteReturn(). This functionality is moved to BytecodeGenerator::BuildAsyncReturn(). This ensures that promise resolution is deferred until all finally blocks are evaluated fully. - Add a new deferred command (CMD_ASYNC_RETURN), which instructs ControlScope to generate return code using BuildAsyncReturn rather than BuildReturn. - Parser has a new `NewReturnStatement()` helper which determines what type of return statement to generate based on the type of function. BUG=v8:5896, v8:4483 R=littledan@chromium.org, neis@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, gsathya@chromium.org ==========
https://codereview.chromium.org/2685683002/diff/80001/src/ast/scopes.h File src/ast/scopes.h (right): https://codereview.chromium.org/2685683002/diff/80001/src/ast/scopes.h#newcod... src/ast/scopes.h:897: Variable* promise_; On 2017/02/10 08:16:59, marja wrote: > Adding these increases the DeclarationScope size with a non-trivial amount... :/ > > Could you measure the size increase in some practical cases so that we can > reason about it, whether it's significant or not? > > Since these aren't needed for all DeclarationScopes, would it be feasible to > create subclasses where these members are? How about something like ``` Variable* generator_object_var() { if (!rare_data_) return nullptr; return rare_data_->generator_object_; } ``` where `rare_data_` gets lazily allocated as-needed? That would add a pointer rather than 2, but it's still simpler than a subclass
https://codereview.chromium.org/2685683002/diff/80001/src/ast/scopes.h File src/ast/scopes.h (right): https://codereview.chromium.org/2685683002/diff/80001/src/ast/scopes.h#newcod... src/ast/scopes.h:897: Variable* promise_; On 2017/02/10 08:20:38, caitp wrote: > On 2017/02/10 08:16:59, marja wrote: > > Adding these increases the DeclarationScope size with a non-trivial amount... > :/ > > > > Could you measure the size increase in some practical cases so that we can > > reason about it, whether it's significant or not? > > > > Since these aren't needed for all DeclarationScopes, would it be feasible to > > create subclasses where these members are? > > How about something like > > ``` > Variable* generator_object_var() { > if (!rare_data_) return nullptr; > return rare_data_->generator_object_; > } > ``` > > where `rare_data_` gets lazily allocated as-needed? That would add a pointer > rather than 2, but it's still simpler than a subclass Sounds good - you can also put this_function_ there, then this CL is memory-neutral for those DeclarationScopes which are not one of those 3 special kinds.
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 caitp@igalia.com 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/2685683002/diff/120001/src/ast/scopes.h File src/ast/scopes.h (right): https://codereview.chromium.org/2685683002/diff/120001/src/ast/scopes.h#newco... src/ast/scopes.h:895: void* operator new(size_t size, Zone* zone) { return zone->New(size); } Okay, here's a "RareData" solution... it does potentially mean that the rare data structure won't be close to the scope itself in its zone, but presumably that could be changed by allocating it early based on function-kind.
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...)
The reason no changes are needed in full-codegen is the "async return" thing is never on the FC/CS pipeline, and there's an assertion which fires if it does end up on that pipeline in ast-numbering.cc. https://codereview.chromium.org/2685683002/diff/80001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2685683002/diff/80001/src/ast/ast.h#newcode918 src/ast/ast.h:918: : public BitField<Type, JumpStatement::kNextBitFieldIndex, 2> {}; On 2017/02/10 08:16:59, marja wrote: > Nit: isn't 1 bit enough? It is, done https://codereview.chromium.org/2685683002/diff/80001/src/ast/scopes.h File src/ast/scopes.h (right): https://codereview.chromium.org/2685683002/diff/80001/src/ast/scopes.h#newcod... src/ast/scopes.h:897: Variable* promise_; On 2017/02/10 08:24:15, marja wrote: > On 2017/02/10 08:20:38, caitp wrote: > > On 2017/02/10 08:16:59, marja wrote: > > > Adding these increases the DeclarationScope size with a non-trivial > amount... > > :/ > > > > > > Could you measure the size increase in some practical cases so that we can > > > reason about it, whether it's significant or not? > > > > > > Since these aren't needed for all DeclarationScopes, would it be feasible to > > > create subclasses where these members are? > > > > How about something like > > > > ``` > > Variable* generator_object_var() { > > if (!rare_data_) return nullptr; > > return rare_data_->generator_object_; > > } > > ``` > > > > where `rare_data_` gets lazily allocated as-needed? That would add a pointer > > rather than 2, but it's still simpler than a subclass > > Sounds good - you can also put this_function_ there, then this CL is > memory-neutral for those DeclarationScopes which are not one of those 3 special > kinds. I doubt either approach would be a huge regression on existing benchmarks, but as noted in another comment, the "RareData" idea is implemented now. https://codereview.chromium.org/2685683002/diff/80001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2685683002/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:2482: temp->set_is_used(); On 2017/02/10 08:16:59, marja wrote: > Should the is_used be moved to DeclareGeneratorObjectVar - is there ever a case > where we would DeclareGeneratorObjectVar but not mark it as used? Fair enough, done https://codereview.chromium.org/2685683002/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:3098: promise->set_is_used(); On 2017/02/10 08:16:59, marja wrote: > Ditto Both moved
lgtm Scope size regressions won't show up in (speed) benchmarks but Scope size has historically been a problem for us (luckily, we've been able to reduce Scope size quite a bit) wrt memory consumption. I think it's also good that we now have a mechanism for adding fields which are needed for very few scopes :)
Ah, wanted to clarify that parser/* and ast/* LGTM
On 2017/02/10 13:36:40, marja wrote: > Ah, wanted to clarify that parser/* and ast/* LGTM Thanks.
The CQ bit was checked by caitp@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, littledan@chromium.org Link to the patchset: https://codereview.chromium.org/2685683002/#ps160001 (title: "also shrink the bitfield for ReturnStatement::Type")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486735995048630,
"parent_rev": "a360134bf1b8aad0d3d7fe72db9bb245b25c7ad3", "commit_rev":
"39642fa2bef2ca151af32fa0f5bfbf357e8914aa"}
Message was sent while issue was closed.
Description was changed from ========== [async-await] (simpler) fix for Return in try/finally in async functions Alternative approach to https://codereview.chromium.org/2667983004/, which does not depend on implicit control flow changes from https://codereview.chromium.org/2664083002 - Remove handling for `async function` from Parser::RewriteReturn(). This functionality is moved to BytecodeGenerator::BuildAsyncReturn(). This ensures that promise resolution is deferred until all finally blocks are evaluated fully. - Add a new deferred command (CMD_ASYNC_RETURN), which instructs ControlScope to generate return code using BuildAsyncReturn rather than BuildReturn. - Parser has a new `NewReturnStatement()` helper which determines what type of return statement to generate based on the type of function. BUG=v8:5896, v8:4483 R=littledan@chromium.org, neis@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, gsathya@chromium.org ========== to ========== [async-await] (simpler) fix for Return in try/finally in async functions Alternative approach to https://codereview.chromium.org/2667983004/, which does not depend on implicit control flow changes from https://codereview.chromium.org/2664083002 - Remove handling for `async function` from Parser::RewriteReturn(). This functionality is moved to BytecodeGenerator::BuildAsyncReturn(). This ensures that promise resolution is deferred until all finally blocks are evaluated fully. - Add a new deferred command (CMD_ASYNC_RETURN), which instructs ControlScope to generate return code using BuildAsyncReturn rather than BuildReturn. - Parser has a new `NewReturnStatement()` helper which determines what type of return statement to generate based on the type of function. BUG=v8:5896, v8:4483 R=littledan@chromium.org, neis@chromium.org, rmcilroy@chromium.org, adamk@chromium.org, gsathya@chromium.org Review-Url: https://codereview.chromium.org/2685683002 Cr-Commit-Position: refs/heads/master@{#43104} Committed: https://chromium.googlesource.com/v8/v8/+/39642fa2bef2ca151af32fa0f5bfbf357e8... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/v8/v8/+/39642fa2bef2ca151af32fa0f5bfbf357e8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
