Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2051423003:
[interpreter] support async functions in Ignition (Closed)
|
Created:
4 years, 6 months ago by caitp (gmail) Modified:
4 years, 6 months ago CC:
oth, rmcilroy, v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[interpreter] support async functions in Ignition
BUG=v8:4483, v8:4907, 618603
LOG=N
R=neis@chromium.org, yangguo@chromium.org, littledan@chromium.org
Committed: https://crrev.com/1a3086623930e5216c02a3ea2c94682f69b58d41
Cr-Commit-Position: refs/heads/master@{#36938}
Patch Set 1 #Patch Set 2 : Revert adding flag to debugger tests, they aren't ready #Patch Set 3 : disable --ignition-generators in some tests #Patch Set 4 : georg's requests #Patch Set 5 : rebase / remove pointless edit to debugger test #
Total comments: 2
Messages
Total messages: 35 (13 generated)
Patchset #3 (id:40001) has been deleted
Description was changed from ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=neis@chromium.org ========== to ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=neis@chromium.org, yangguo@chromium.org, littledan@chromium.org ==========
caitpotter88@gmail.com changed reviewers: + littledan@chromium.org, yangguo@chromium.org
PTAL Fix for fuzz failure + enable --ignition-generators on some of the async function tests
Thanks for the quick fix. I think --ignition-generators will soon be on by default (i.e., --ignition will always use ignition generators) so I don't think it's necessary to check in this flag flip on anything besides the regression test. Otherwise looks good.
On 2016/06/10 17:07:17, Dan Ehrenberg wrote: > Thanks for the quick fix. I think --ignition-generators will soon be on by > default (i.e., --ignition will always use ignition generators) so I don't think > it's necessary to check in this flag flip on anything besides the regression > test. Otherwise looks good. By "flipped-on by default", I assume you mean the flag will still exist, and just default to "true", right? In that case, I could just add --no-ignition-generators to the debugger tests until v8:5099 is fixed.
On 2016/06/10 at 17:13:31, caitpotter88 wrote: > On 2016/06/10 17:07:17, Dan Ehrenberg wrote: > > Thanks for the quick fix. I think --ignition-generators will soon be on by > > default (i.e., --ignition will always use ignition generators) so I don't think > > it's necessary to check in this flag flip on anything besides the regression > > test. Otherwise looks good. > > By "flipped-on by default", I assume you mean the flag will still exist, and just default to "true", right? In that case, I could just add --no-ignition-generators to the debugger tests until v8:5099 is fixed. Sounds like a good idea to me.
Description was changed from ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=neis@chromium.org, yangguo@chromium.org, littledan@chromium.org ========== to ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=rmcilroy@chromium.org, neis@chromium.org, yangguo@chromium.org, littledan@chromium.org ==========
caitpotter88@gmail.com changed reviewers: + rmcilroy@chromium.org
Seems good to me (but we should make sure to remove this once the bug is fixed!)
Scanning the code base, I see a couple other callsites of IsGeneratorFunction
that should probably be updated:
- src/crankshaft/hydrogen.cc line 161 (to avoid trying to compile generators)
- src/objects.cc line 12241 in JSFunction::SetPrototype (not sure exactly what
this is getting at--I thought that (async function() {}).prototype = undefined
would hit the DCHECK in this function but it doesn't seem to)
Think you could address these in a follow-on patch?
lgtm
On 2016/06/10 17:41:14, Dan Ehrenberg wrote:
> Seems good to me (but we should make sure to remove this once the bug is
fixed!)
>
> Scanning the code base, I see a couple other callsites of IsGeneratorFunction
> that should probably be updated:
> - src/crankshaft/hydrogen.cc line 161 (to avoid trying to compile generators)
> - src/objects.cc line 12241 in JSFunction::SetPrototype (not sure exactly what
> this is getting at--I thought that (async function() {}).prototype = undefined
> would hit the DCHECK in this function but it doesn't seem to)
> Think you could address these in a follow-on patch?
sounds good to me
On 2016/06/10 17:41:14, Dan Ehrenberg wrote:
> Seems good to me (but we should make sure to remove this once the bug is
fixed!)
>
> Scanning the code base, I see a couple other callsites of IsGeneratorFunction
> that should probably be updated:
> - src/crankshaft/hydrogen.cc line 161 (to avoid trying to compile generators)
Generators and Async functions have optimizations disabled in factory.cc, which
should affect hydrogen.cc line ~171. Ditto for generators.
> - src/objects.cc line 12241 in JSFunction::SetPrototype (not sure exactly what
> this is getting at--I thought that (async function() {}).prototype = undefined
> would hit the DCHECK in this function but it doesn't seem to)
The DCHECK isn't hit because the FunctionPrototype accessor info is never
installed on async function maps, due to the FUNCTION_WITHOUT_PROTOTYPE mode.
So I'm not sure either of these really need any work just yet
On 2016/06/10 at 19:51:46, caitpotter88 wrote:
> On 2016/06/10 17:41:14, Dan Ehrenberg wrote:
> > Seems good to me (but we should make sure to remove this once the bug is
fixed!)
> >
> > Scanning the code base, I see a couple other callsites of
IsGeneratorFunction
> > that should probably be updated:
> > - src/crankshaft/hydrogen.cc line 161 (to avoid trying to compile
generators)
>
> Generators and Async functions have optimizations disabled in factory.cc,
which should affect hydrogen.cc line ~171. Ditto for generators.
>
> > - src/objects.cc line 12241 in JSFunction::SetPrototype (not sure exactly
what
> > this is getting at--I thought that (async function() {}).prototype =
undefined
> > would hit the DCHECK in this function but it doesn't seem to)
>
> The DCHECK isn't hit because the FunctionPrototype accessor info is never
installed on async function maps, due to the FUNCTION_WITHOUT_PROTOTYPE mode.
>
>
> So I'm not sure either of these really need any work just yet
Sounds like both paths should DCHECK fail if passed in an IsResumableFunction,
right?
On 2016/06/10 17:13:31, caitp wrote: > On 2016/06/10 17:07:17, Dan Ehrenberg wrote: > > Thanks for the quick fix. I think --ignition-generators will soon be on by > > default (i.e., --ignition will always use ignition generators) so I don't > think > > it's necessary to check in this flag flip on anything besides the regression > > test. Otherwise looks good. > > By "flipped-on by default", I assume you mean the flag will still exist, and > just default to "true", right? In that case, I could just add > --no-ignition-generators to the debugger tests until v8:5099 is fixed. I'd prefer no flag here, so that we get reminded of the problem when we turn on ignition-generators by default (if we don't have a fix by then, we'll have to add a mjsunit.status entry). Also, in ther parser, can you update the comment around line 700 to something like the following? // This is a regular call. Fall through to the ordinary function prologue, // after which we will run into the generator object creation and other extra // code inserted by the parser. LGTM then.
On 2016/06/13 08:35:15, neis wrote: > On 2016/06/10 17:13:31, caitp wrote: > > On 2016/06/10 17:07:17, Dan Ehrenberg wrote: > > > Thanks for the quick fix. I think --ignition-generators will soon be on by > > > default (i.e., --ignition will always use ignition generators) so I don't > > think > > > it's necessary to check in this flag flip on anything besides the regression > > > test. Otherwise looks good. > > > > By "flipped-on by default", I assume you mean the flag will still exist, and > > just default to "true", right? In that case, I could just add > > --no-ignition-generators to the debugger tests until v8:5099 is fixed. > > I'd prefer no flag here, so that we get reminded of the problem when we turn on > ignition-generators by default (if we don't have a fix by then, we'll have to > add a mjsunit.status entry). > > Also, in ther parser, can you update the comment around line 700 to something > like the following? > > // This is a regular call. Fall through to the ordinary function prologue, > // after which we will run into the generator object creation and other extra > // code inserted by the parser. Sorry, in the bytecode-generator, not in the parser.
On 2016/06/13 09:02:36, neis wrote: > On 2016/06/13 08:35:15, neis wrote: > > On 2016/06/10 17:13:31, caitp wrote: > > > On 2016/06/10 17:07:17, Dan Ehrenberg wrote: > > > > Thanks for the quick fix. I think --ignition-generators will soon be on by > > > > default (i.e., --ignition will always use ignition generators) so I don't > > > think > > > > it's necessary to check in this flag flip on anything besides the > regression > > > > test. Otherwise looks good. > > > > > > By "flipped-on by default", I assume you mean the flag will still exist, and > > > just default to "true", right? In that case, I could just add > > > --no-ignition-generators to the debugger tests until v8:5099 is fixed. > > > > I'd prefer no flag here, so that we get reminded of the problem when we turn > on > > ignition-generators by default (if we don't have a fix by then, we'll have to > > add a mjsunit.status entry). > > > > Also, in ther parser, can you update the comment around line 700 to something > > like the following? > > > > // This is a regular call. Fall through to the ordinary function prologue, > > // after which we will run into the generator object creation and other > extra > > // code inserted by the parser. > > Sorry, in the bytecode-generator, not in the parser. lgtm.
On 2016/06/13 08:35:15, neis wrote: > On 2016/06/10 17:13:31, caitp wrote: > > On 2016/06/10 17:07:17, Dan Ehrenberg wrote: > > > Thanks for the quick fix. I think --ignition-generators will soon be on by > > > default (i.e., --ignition will always use ignition generators) so I don't > > think > > > it's necessary to check in this flag flip on anything besides the regression > > > test. Otherwise looks good. > > > > By "flipped-on by default", I assume you mean the flag will still exist, and > > just default to "true", right? In that case, I could just add > > --no-ignition-generators to the debugger tests until v8:5099 is fixed. > > I'd prefer no flag here, so that we get reminded of the problem when we turn on > ignition-generators by default (if we don't have a fix by then, we'll have to > add a mjsunit.status entry). > > Also, in ther parser, can you update the comment around line 700 to something > like the following? > > // This is a regular call. Fall through to the ordinary function prologue, > // after which we will run into the generator object creation and other extra > // code inserted by the parser. > > LGTM then. done
On 2016/06/10 21:06:31, Dan Ehrenberg wrote:
> On 2016/06/10 at 19:51:46, caitpotter88 wrote:
> > On 2016/06/10 17:41:14, Dan Ehrenberg wrote:
> > > Seems good to me (but we should make sure to remove this once the bug is
> fixed!)
> > >
> > > Scanning the code base, I see a couple other callsites of
> IsGeneratorFunction
> > > that should probably be updated:
> > > - src/crankshaft/hydrogen.cc line 161 (to avoid trying to compile
> generators)
> >
> > Generators and Async functions have optimizations disabled in factory.cc,
> which should affect hydrogen.cc line ~171. Ditto for generators.
> >
> > > - src/objects.cc line 12241 in JSFunction::SetPrototype (not sure exactly
> what
> > > this is getting at--I thought that (async function() {}).prototype =
> undefined
> > > would hit the DCHECK in this function but it doesn't seem to)
> >
> > The DCHECK isn't hit because the FunctionPrototype accessor info is never
> installed on async function maps, due to the FUNCTION_WITHOUT_PROTOTYPE mode.
> >
> >
> > So I'm not sure either of these really need any work just yet
>
> Sounds like both paths should DCHECK fail if passed in an IsResumableFunction,
> right?
I don't think crankshaft will (at least not around that area of code, far as I
can see), but the SetPrototype accessor will. But this is okay, IMHO, because
SetPrototype will not be called with an async function at the moment, because of
the way async function maps are set up. It doesn't look like it's reachable via
the API or script code either, so leaving the DCHECK as-is seems fine to me.
Description was changed from ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=rmcilroy@chromium.org, neis@chromium.org, yangguo@chromium.org, littledan@chromium.org ========== to ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=neis@chromium.org, yangguo@chromium.org, littledan@chromium.org ==========
caitpotter88@gmail.com changed reviewers: + bmeurer@chromium.org
+more src/interpreter OWNERs
+more src/interpreter OWNERs
caitpotter88@gmail.com changed reviewers: + mstarzinger@chromium.org, oth@chromium.org
LGTM with comment. https://codereview.chromium.org/2051423003/diff/100001/src/globals.h File src/globals.h (right): https://codereview.chromium.org/2051423003/diff/100001/src/globals.h#newcode1028 src/globals.h:1028: return IsGeneratorFunction(kind) || IsAsyncFunction(kind); I'm really not sure this helper function adds to the overall readability, as that is yet another term to remember. I'd prefer to have just IsGeneratorFunction(kind)||IsAsyncFunction(kind) whenever something applies to both, that makes it immediately obvious that the particular code applies to both generators and async functions w/o having to lookup what a "resumable function" might be. Plus it makes it easier to spot code related to async function by just grepping for AsyncFunction. But I see that this "resumable" predicate is already being used in other places. So fine for this CL, but maybe we can have a cleanup CL afterwards to remove the "resumable" terminology and explicitly say "generator" and "async function" instead?
https://codereview.chromium.org/2051423003/diff/100001/src/globals.h File src/globals.h (right): https://codereview.chromium.org/2051423003/diff/100001/src/globals.h#newcode1028 src/globals.h:1028: return IsGeneratorFunction(kind) || IsAsyncFunction(kind); On 2016/06/13 17:13:47, Benedikt Meurer wrote: > I'm really not sure this helper function adds to the overall readability, as > that is yet another term to remember. I'd prefer to have just > IsGeneratorFunction(kind)||IsAsyncFunction(kind) whenever something applies to > both, that makes it immediately obvious that the particular code applies to both > generators and async functions w/o having to lookup what a "resumable function" > might be. Plus it makes it easier to spot code related to async function by just > grepping for AsyncFunction. > > But I see that this "resumable" predicate is already being used in other places. > So fine for this CL, but maybe we can have a cleanup CL afterwards to remove the > "resumable" terminology and explicitly say "generator" and "async function" > instead? I generally agree, but it is sort of mixed messaging between different reviewers. If this sort of pattern comes up in the future, there should probably be project-wide guidelines for it
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org, littledan@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2051423003/#ps100001 (title: "rebase / remove pointless edit to debugger test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051423003/100001
Message was sent while issue was closed.
Description was changed from ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=neis@chromium.org, yangguo@chromium.org, littledan@chromium.org ========== to ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=neis@chromium.org, yangguo@chromium.org, littledan@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=neis@chromium.org, yangguo@chromium.org, littledan@chromium.org ========== to ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=neis@chromium.org, yangguo@chromium.org, littledan@chromium.org Committed: https://crrev.com/1a3086623930e5216c02a3ea2c94682f69b58d41 Cr-Commit-Position: refs/heads/master@{#36938} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1a3086623930e5216c02a3ea2c94682f69b58d41 Cr-Commit-Position: refs/heads/master@{#36938}
Message was sent while issue was closed.
On 2016/06/13 at 17:13:47, bmeurer wrote: > LGTM with comment. > > https://codereview.chromium.org/2051423003/diff/100001/src/globals.h > File src/globals.h (right): > > https://codereview.chromium.org/2051423003/diff/100001/src/globals.h#newcode1028 > src/globals.h:1028: return IsGeneratorFunction(kind) || IsAsyncFunction(kind); > I'm really not sure this helper function adds to the overall readability, as that is yet another term to remember. I'd prefer to have just IsGeneratorFunction(kind)||IsAsyncFunction(kind) whenever something applies to both, that makes it immediately obvious that the particular code applies to both generators and async functions w/o having to lookup what a "resumable function" might be. Plus it makes it easier to spot code related to async function by just grepping for AsyncFunction. > > But I see that this "resumable" predicate is already being used in other places. So fine for this CL, but maybe we can have a cleanup CL afterwards to remove the "resumable" terminology and explicitly say "generator" and "async function" instead? Most cases where the code base needs to do something like this, it needs to check whether something is implemented using a generator, rather than whether it surfaces as a generator. I like having a single term to unify this concept. (Personally, I'd be fine with "generator", and use something like "generator function" for the more specific case, but "resumable" also works.) We currently have only generators and async functions, but in the future we may have async generators, which are neither ordinary generators nor ordinary async functions, and it will start to become unwieldy to include all three.
Message was sent while issue was closed.
Description was changed from ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=neis@chromium.org, yangguo@chromium.org, littledan@chromium.org Committed: https://crrev.com/1a3086623930e5216c02a3ea2c94682f69b58d41 Cr-Commit-Position: refs/heads/master@{#36938} ========== to ========== [interpreter] support async functions in Ignition BUG=v8:4483, v8:4907, 618603 LOG=N R=neis@chromium.org, yangguo@chromium.org, littledan@chromium.org Committed: https://crrev.com/1a3086623930e5216c02a3ea2c94682f69b58d41 Cr-Commit-Position: refs/heads/master@{#36938} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
