Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2655623005:
[parser] Skipping inner funcs: add info about variables. (Closed)
Description[parser] Skipping inner funcs: add info about variables.
- Declaring a variable called "this" for preparsed functions was unnecessary;
DeclarationScope ctor already adds the variable.
- "arguments" for preparsed scopes need to be declared after parsing the
function, like it's done in the parser.
- Now arguments_ can be the dummy variable, so adapted code to it.
- A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was
incomplete; it had added ParserBase::ParseFunctionBody but
PreParser::ParseFunction didn't call it. This CL completes that work. This is
needed for getting "arguments" declared properly for preparsed functions.
- AllocateVariablesRecursively is already called for preparsed scopes (without
this CL, that is), and it bails out early. However, before the bailout it used
to dcheck num_stack_slots_ == 0; that is no longer true since we've done scope
analysis for preparsed scopes.
- Test fix: we cannot have any lazy inner functions in the test, except the
topmost lazy inner function. Such functions would also be lazy in the parser
case, and the parser would just throw away their variables. Then the test
tries to verify the preparsed data against the scopes without variables and fails.
- Disabled a test w/ a sloppy block function, will get that working again in the
upcoming CLs.
BUG=v8:5516
Review-Url: https://codereview.chromium.org/2655623005
Cr-Commit-Position: refs/heads/master@{#42685}
Committed: https://chromium.googlesource.com/v8/v8/+/d4507a6cf9ca4312d0f0b143f7ef5f6c7802ac29
Patch Set 1 #Patch Set 2 : alt fix #
Total comments: 4
Patch Set 3 : create variables only when the flag is on #
Total comments: 2
Messages
Total messages: 25 (14 generated)
The CQ bit was checked by marja@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [parser] Skipping inner funcs: add info about variables. - Declaring a variable called "this" for preparsed functions was unnecessary; DeclarationScope ctor already adds the variable. - "arguments" for preparsed scopes need to be declared after parsing the function, like it's done in the parser. - A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was incomplete; it had added ParserBase::ParseFunctionBody but PreParser::ParseFunction didn't call it. This CL completes that work. This is needed for getting "arguments" declared properly for preparsed functions. - Test fix: we cannot have any lazy inner functions in the test, except the topmost lazy inner function. Such functions would also be lazy in the parser case, and the parser would just throw away their variables. Then the test tries to verify the preparsed data against. - Disabled a test w/ a sloppy block function, will get that working again in the upcoming CLs. BUG=v8:5516 ========== to ========== [parser] Skipping inner funcs: add info about variables. - Declaring a variable called "this" for preparsed functions was unnecessary; DeclarationScope ctor already adds the variable. - "arguments" for preparsed scopes need to be declared after parsing the function, like it's done in the parser. - A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was incomplete; it had added ParserBase::ParseFunctionBody but PreParser::ParseFunction didn't call it. This CL completes that work. This is needed for getting "arguments" declared properly for preparsed functions. - AllocateVariablesRecursively is already called for preparsed scopes (without this CL, that is), and it bails out early. However, before the bailout it used to dcheck num_stack_slots_ == 0; that is no longer true since we've done scope analysis for preparsed scopes. - Test fix: we cannot have any lazy inner functions in the test, except the topmost lazy inner function. Such functions would also be lazy in the parser case, and the parser would just throw away their variables. Then the test tries to verify the preparsed data against. - Disabled a test w/ a sloppy block function, will get that working again in the upcoming CLs. BUG=v8:5516 ==========
Description was changed from ========== [parser] Skipping inner funcs: add info about variables. - Declaring a variable called "this" for preparsed functions was unnecessary; DeclarationScope ctor already adds the variable. - "arguments" for preparsed scopes need to be declared after parsing the function, like it's done in the parser. - A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was incomplete; it had added ParserBase::ParseFunctionBody but PreParser::ParseFunction didn't call it. This CL completes that work. This is needed for getting "arguments" declared properly for preparsed functions. - AllocateVariablesRecursively is already called for preparsed scopes (without this CL, that is), and it bails out early. However, before the bailout it used to dcheck num_stack_slots_ == 0; that is no longer true since we've done scope analysis for preparsed scopes. - Test fix: we cannot have any lazy inner functions in the test, except the topmost lazy inner function. Such functions would also be lazy in the parser case, and the parser would just throw away their variables. Then the test tries to verify the preparsed data against. - Disabled a test w/ a sloppy block function, will get that working again in the upcoming CLs. BUG=v8:5516 ========== to ========== [parser] Skipping inner funcs: add info about variables. - Declaring a variable called "this" for preparsed functions was unnecessary; DeclarationScope ctor already adds the variable. - "arguments" for preparsed scopes need to be declared after parsing the function, like it's done in the parser. - A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was incomplete; it had added ParserBase::ParseFunctionBody but PreParser::ParseFunction didn't call it. This CL completes that work. This is needed for getting "arguments" declared properly for preparsed functions. - AllocateVariablesRecursively is already called for preparsed scopes (without this CL, that is), and it bails out early. However, before the bailout it used to dcheck num_stack_slots_ == 0; that is no longer true since we've done scope analysis for preparsed scopes. - Test fix: we cannot have any lazy inner functions in the test, except the topmost lazy inner function. Such functions would also be lazy in the parser case, and the parser would just throw away their variables. Then the test tries to verify the preparsed data against. - Disabled a test w/ a sloppy block function, will get that working again in the upcoming CLs. BUG=v8:5516 ==========
marja@chromium.org changed reviewers: + vogelheim@chromium.org
ptal
jochen@chromium.org changed reviewers: + jochen@chromium.org
have you considered landing all the items from the CL description as individual patches? also, there's one block (added inline comment) that I can't connect to any of the items in the cl description - please explain! https://codereview.chromium.org/2655623005/diff/20001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2655623005/diff/20001/src/ast/scopes.cc#newco... src/ast/scopes.cc:1083: Variable* var = LookupLocal(name); can you please explain this change?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
All these changes are needed to get even the simplest test to pass; I could split the CL but none of the changes wouldn't be observable until the last one is in. https://codereview.chromium.org/2655623005/diff/20001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2655623005/diff/20001/src/ast/scopes.cc#newco... src/ast/scopes.cc:1083: Variable* var = LookupLocal(name); On 2017/01/25 16:34:14, jochen (travelling til Feb 4) wrote: > can you please explain this change? This is kinda covered by the title line; in order to add data about Variables we need to create Variables. I was going a bit of back and forth wrt should I put this change behind the flag. I decided not to but now I'm having second thoughts. Btw, now all DeclareName stuff and everything hanging from it (dummy variables and such) are dead code and I should've removed them if I took the non-flag approach... But now I think I should put this behind the flag. Opinions?
Description was changed from ========== [parser] Skipping inner funcs: add info about variables. - Declaring a variable called "this" for preparsed functions was unnecessary; DeclarationScope ctor already adds the variable. - "arguments" for preparsed scopes need to be declared after parsing the function, like it's done in the parser. - A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was incomplete; it had added ParserBase::ParseFunctionBody but PreParser::ParseFunction didn't call it. This CL completes that work. This is needed for getting "arguments" declared properly for preparsed functions. - AllocateVariablesRecursively is already called for preparsed scopes (without this CL, that is), and it bails out early. However, before the bailout it used to dcheck num_stack_slots_ == 0; that is no longer true since we've done scope analysis for preparsed scopes. - Test fix: we cannot have any lazy inner functions in the test, except the topmost lazy inner function. Such functions would also be lazy in the parser case, and the parser would just throw away their variables. Then the test tries to verify the preparsed data against. - Disabled a test w/ a sloppy block function, will get that working again in the upcoming CLs. BUG=v8:5516 ========== to ========== [parser] Skipping inner funcs: add info about variables. - Declaring a variable called "this" for preparsed functions was unnecessary; DeclarationScope ctor already adds the variable. - "arguments" for preparsed scopes need to be declared after parsing the function, like it's done in the parser. - A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was incomplete; it had added ParserBase::ParseFunctionBody but PreParser::ParseFunction didn't call it. This CL completes that work. This is needed for getting "arguments" declared properly for preparsed functions. - AllocateVariablesRecursively is already called for preparsed scopes (without this CL, that is), and it bails out early. However, before the bailout it used to dcheck num_stack_slots_ == 0; that is no longer true since we've done scope analysis for preparsed scopes. - Test fix: we cannot have any lazy inner functions in the test, except the topmost lazy inner function. Such functions would also be lazy in the parser case, and the parser would just throw away their variables. Then the test tries to verify the preparsed data against the scopes without variables and fails. - Disabled a test w/ a sloppy block function, will get that working again in the upcoming CLs. BUG=v8:5516 ==========
no flags gives you more coverage, so lgtm
On 2017/01/25 17:20:55, jochen (travelling til Feb 4) wrote: > no flags gives you more coverage, so lgtm I'd like to understand what this means, exactly. Marja said that the dummy variables are "dead code" without the flag. Either that means they run and don't have any effect (in which case "more coverage" is true but it seems bad to run more code than necessary) or they don't run, in which case there's no need for a flag guard, but then we also don't get "more coverage". Just want to make sure I understand how this feature's flag-guarding works.
my concern was that the new code might have a perf hit, and I thought that if we enable this code right now instead of waiting, we'll see it now (as opposed to when we flip the flag, and a large chunk of new code might be responsible for the regression)
If I go for the solution of not putting the DeclareLocal behind the flag, then I'll of course remove the dead code (just forgot to do it in this patch set, since I was going back and forth with the decision). However, I think it's better to put it behind the flag: constructing the Variables increases memory consumption & takes time, and we're still nowhere close to being able to use that information. So we'd just spend time and memory doing stuff we cannot use (yet). It's expected that we're a bit slower w/ the flag (thought it might not be noticeable in any benchmarks), but we should get the performance back when we're able to skip over inner functions.
Description was changed from ========== [parser] Skipping inner funcs: add info about variables. - Declaring a variable called "this" for preparsed functions was unnecessary; DeclarationScope ctor already adds the variable. - "arguments" for preparsed scopes need to be declared after parsing the function, like it's done in the parser. - A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was incomplete; it had added ParserBase::ParseFunctionBody but PreParser::ParseFunction didn't call it. This CL completes that work. This is needed for getting "arguments" declared properly for preparsed functions. - AllocateVariablesRecursively is already called for preparsed scopes (without this CL, that is), and it bails out early. However, before the bailout it used to dcheck num_stack_slots_ == 0; that is no longer true since we've done scope analysis for preparsed scopes. - Test fix: we cannot have any lazy inner functions in the test, except the topmost lazy inner function. Such functions would also be lazy in the parser case, and the parser would just throw away their variables. Then the test tries to verify the preparsed data against the scopes without variables and fails. - Disabled a test w/ a sloppy block function, will get that working again in the upcoming CLs. BUG=v8:5516 ========== to ========== [parser] Skipping inner funcs: add info about variables. - Declaring a variable called "this" for preparsed functions was unnecessary; DeclarationScope ctor already adds the variable. - "arguments" for preparsed scopes need to be declared after parsing the function, like it's done in the parser. - Now arguments_ can be the dummy variable, so adapted code to it. - A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was incomplete; it had added ParserBase::ParseFunctionBody but PreParser::ParseFunction didn't call it. This CL completes that work. This is needed for getting "arguments" declared properly for preparsed functions. - AllocateVariablesRecursively is already called for preparsed scopes (without this CL, that is), and it bails out early. However, before the bailout it used to dcheck num_stack_slots_ == 0; that is no longer true since we've done scope analysis for preparsed scopes. - Test fix: we cannot have any lazy inner functions in the test, except the topmost lazy inner function. Such functions would also be lazy in the parser case, and the parser would just throw away their variables. Then the test tries to verify the preparsed data against the scopes without variables and fails. - Disabled a test w/ a sloppy block function, will get that working again in the upcoming CLs. BUG=v8:5516 ==========
lgtm, but please look at the test comment. Not sure if bug or if I just misudnerstood the code. :) https://codereview.chromium.org/2655623005/diff/20001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (left): https://codereview.chromium.org/2655623005/diff/20001/test/cctest/test-parsin... test/cctest/test-parsing.cc:9025: {"", "if (true) { function f1() {} }"}, Is this the "Disabled a test w/ ..." part? https://codereview.chromium.org/2655623005/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2655623005/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:8722: CHECK_EQ(data->backing_store_[index++], variable_count); Why does this work? The way I read Scope::CollectVariableData it will add all locals inside the recursion over the inner-scope sub-tree, so I'd expect it to have - at each scope - all variables of all inner scopes. This one only counts local variables in the current scope, and compares it to the value computed as above. Am I missing something? Also, is there a test case that would distinguish those two cases?
thanks for reviewing! https://codereview.chromium.org/2655623005/diff/20001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (left): https://codereview.chromium.org/2655623005/diff/20001/test/cctest/test-parsin... test/cctest/test-parsing.cc:9025: {"", "if (true) { function f1() {} }"}, On 2017/01/26 09:34:35, vogelheim wrote: > Is this the "Disabled a test w/ ..." part? Yeah, this is the sloppy block function. https://codereview.chromium.org/2655623005/diff/40001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/2655623005/diff/40001/test/cctest/test-parsin... test/cctest/test-parsing.cc:8722: CHECK_EQ(data->backing_store_[index++], variable_count); On 2017/01/26 09:34:35, vogelheim wrote: > Why does this work? The way I read Scope::CollectVariableData it will add all > locals inside the recursion over the inner-scope sub-tree, so I'd expect it to > have - at each scope - all variables of all inner scopes. > > This one only counts local variables in the current scope, and compares it to > the value computed as above. Am I missing something? Also, is there a test case > that would distinguish those two cases? Offline discussion: the scope contains only the variables in that scope, and then CollectVariableData recurses into the subscope which adds a new ScopeScope to the data.
The CQ bit was checked by marja@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2655623005/#ps40001 (title: "create variables only when the flag is on")
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": 40001, "attempt_start_ts": 1485423668334630,
"parent_rev": "d24ce605c99f5b45e596e88931127d6ce6d0d27a", "commit_rev":
"d4507a6cf9ca4312d0f0b143f7ef5f6c7802ac29"}
Message was sent while issue was closed.
Description was changed from ========== [parser] Skipping inner funcs: add info about variables. - Declaring a variable called "this" for preparsed functions was unnecessary; DeclarationScope ctor already adds the variable. - "arguments" for preparsed scopes need to be declared after parsing the function, like it's done in the parser. - Now arguments_ can be the dummy variable, so adapted code to it. - A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was incomplete; it had added ParserBase::ParseFunctionBody but PreParser::ParseFunction didn't call it. This CL completes that work. This is needed for getting "arguments" declared properly for preparsed functions. - AllocateVariablesRecursively is already called for preparsed scopes (without this CL, that is), and it bails out early. However, before the bailout it used to dcheck num_stack_slots_ == 0; that is no longer true since we've done scope analysis for preparsed scopes. - Test fix: we cannot have any lazy inner functions in the test, except the topmost lazy inner function. Such functions would also be lazy in the parser case, and the parser would just throw away their variables. Then the test tries to verify the preparsed data against the scopes without variables and fails. - Disabled a test w/ a sloppy block function, will get that working again in the upcoming CLs. BUG=v8:5516 ========== to ========== [parser] Skipping inner funcs: add info about variables. - Declaring a variable called "this" for preparsed functions was unnecessary; DeclarationScope ctor already adds the variable. - "arguments" for preparsed scopes need to be declared after parsing the function, like it's done in the parser. - Now arguments_ can be the dummy variable, so adapted code to it. - A previous refactoring CL ( https://codereview.chromium.org/2638333002 ) was incomplete; it had added ParserBase::ParseFunctionBody but PreParser::ParseFunction didn't call it. This CL completes that work. This is needed for getting "arguments" declared properly for preparsed functions. - AllocateVariablesRecursively is already called for preparsed scopes (without this CL, that is), and it bails out early. However, before the bailout it used to dcheck num_stack_slots_ == 0; that is no longer true since we've done scope analysis for preparsed scopes. - Test fix: we cannot have any lazy inner functions in the test, except the topmost lazy inner function. Such functions would also be lazy in the parser case, and the parser would just throw away their variables. Then the test tries to verify the preparsed data against the scopes without variables and fails. - Disabled a test w/ a sloppy block function, will get that working again in the upcoming CLs. BUG=v8:5516 Review-Url: https://codereview.chromium.org/2655623005 Cr-Commit-Position: refs/heads/master@{#42685} Committed: https://chromium.googlesource.com/v8/v8/+/d4507a6cf9ca4312d0f0b143f7ef5f6c780... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/d4507a6cf9ca4312d0f0b143f7ef5f6c780... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
