Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2400613003:
PreParsing inner functions: Fix declaration-only variables, part 2. (Closed)
DescriptionPreParsing inner functions: Fix declaration-only variables, part 2.
If an inner function only declares a variable but doesn't use it, Parser
and PreParser produced different unresolved variables, and that confused
the pessimistic context allocation.
This is continuation to https://codereview.chromium.org/2388183003/
This CL fixes more complicated declarations (which are not just one
identifier). For this, PreParser needs to accumulate identifiers used
in expressions.
In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that
we get clusterfuzz coverage for it.
BUG=chromium:650969, v8:5501
Committed: https://crrev.com/e474e5ffc83cddc0cc1ff7cc9d247f90cd827bfd
Cr-Commit-Position: refs/heads/master@{#40112}
Patch Set 1 #Patch Set 2 : more tests and fixes #Patch Set 3 : cleanup #Patch Set 4 : beautify #Patch Set 5 : flag on - just for try jobs - will land w/ flag off #Patch Set 6 : rebased #
Total comments: 7
Patch Set 7 : flag off + code review (adamk@) #Patch Set 8 : rebased #
Messages
Total messages: 28 (18 generated)
Description was changed from ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This CL fixes more complicated declarations (which are not just one identifiers). For this, PreParser needs to accumulate identifiers used in expressions. BUG=chromium:650969 ========== to ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifiers). For this, PreParser needs to accumulate identifiers used in expressions. BUG=chromium:650969 ==========
marja@chromium.org changed reviewers: + adamk@chromium.org
Description was changed from ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifiers). For this, PreParser needs to accumulate identifiers used in expressions. BUG=chromium:650969 ========== to ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifiers). For this, PreParser needs to accumulate identifiers used in expressions. In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that we get clusterfuzz coverage for it. BUG=chromium:650969 ==========
Description was changed from ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifiers). For this, PreParser needs to accumulate identifiers used in expressions. In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that we get clusterfuzz coverage for it. BUG=chromium:650969 ========== to ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifiers). For this, PreParser needs to accumulate identifiers used in expressions. In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that we get clusterfuzz coverage for it. BUG=chromium:650969 ==========
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 ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifiers). For this, PreParser needs to accumulate identifiers used in expressions. In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that we get clusterfuzz coverage for it. BUG=chromium:650969 ========== to ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifier). For this, PreParser needs to accumulate identifiers used in expressions. In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that we get clusterfuzz coverage for it. BUG=chromium:650969 ==========
adamk ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
Ahem, the tests that fail on try bots didn't fail locally, also Stderr: Exception thrown during bootstrapping Extension or internal compilation error: Unexpected token % in v8/gc at line 1. is quite suspicious; this CL shouldn't affect anything related to natives. --> I'll debug what's up.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
Alright, debugged the failure. It's independent of this CL, and also happens in master w/ the FLAG_lazy_inner_functions. Afaics the problem is that context-allocating a variable unnecessarily disables some Turbofan optimizations. So I'd like to land this CL (with the flag off ofc), since this it going into the direction of PreParser actually declaring (some) variables which will lead into fixing the problem. Before the flag is enabled, the problem needs to be fixed ofc.
https://codereview.chromium.org/2400613003/diff/90001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2400613003/diff/90001/src/ast/scopes.cc#newco... src/ast/scopes.cc:1010: if (next != nullptr && next->raw_name() == name) { Why was this null check (and the one above) not needed before? https://codereview.chromium.org/2400613003/diff/90001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2400613003/diff/90001/src/parsing/parser.cc#n... src/parsing/parser.cc:127: parser_->reusable_preparser_->factory()->set_zone(temp_zone); Seems a little weird to set this zone, but not the one for ast_value_factory...I'm sure I'm missing something, but what is it? https://codereview.chromium.org/2400613003/diff/90001/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2400613003/diff/90001/src/parsing/preparser.h... src/parsing/preparser.h:542: expression.AddIdentifier(identifier.string_, zone_); Which case is this needed by? Does this have to do with how we canonicalize object literal property names? If so a comment would be valuable.
thanks for having a look! https://codereview.chromium.org/2400613003/diff/90001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2400613003/diff/90001/src/ast/scopes.cc#newco... src/ast/scopes.cc:1010: if (next != nullptr && next->raw_name() == name) { On 2016/10/07 18:35:55, adamk wrote: > Why was this null check (and the one above) not needed before? - This function is only used by the PreParser - PreParser shouldn't RemoveUnresolved(sth_that_doesnt_exist) even after this CL, so it's strictly not needed - However, this code is much more sensemaking w/ the nullptr check so it makes sense to have it - I'll add a DCHECK to PreParser to check the return value. https://codereview.chromium.org/2400613003/diff/90001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2400613003/diff/90001/src/parsing/parser.cc#n... src/parsing/parser.cc:127: parser_->reusable_preparser_->factory()->set_zone(temp_zone); On 2016/10/07 18:35:55, adamk wrote: > Seems a little weird to set this zone, but not the one for > ast_value_factory...I'm sure I'm missing something, but what is it? Offline discussion: - AstValueFactories always use the main Zone - PreParserFactory ctor has a weird signature because it needs to match AstNodeFactory https://codereview.chromium.org/2400613003/diff/90001/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2400613003/diff/90001/src/parsing/preparser.h... src/parsing/preparser.h:542: expression.AddIdentifier(identifier.string_, zone_); On 2016/10/07 18:35:55, adamk wrote: > Which case is this needed by? Does this have to do with how we canonicalize > object literal property names? If so a comment would be valuable. Going to add a comment: This is needed for object literal property names. Property names are normalized to string literals during object literal parsing.
lgtm, thanks
Description was changed from ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifier). For this, PreParser needs to accumulate identifiers used in expressions. In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that we get clusterfuzz coverage for it. BUG=chromium:650969 ========== to ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifier). For this, PreParser needs to accumulate identifiers used in expressions. In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that we get clusterfuzz coverage for it. BUG=chromium:650969, v8:5501 ==========
thanks for review! https://codereview.chromium.org/2400613003/diff/90001/src/parsing/preparser.h File src/parsing/preparser.h (right): https://codereview.chromium.org/2400613003/diff/90001/src/parsing/preparser.h... src/parsing/preparser.h:542: expression.AddIdentifier(identifier.string_, zone_); On 2016/10/07 19:00:27, marja wrote: > On 2016/10/07 18:35:55, adamk wrote: > > Which case is this needed by? Does this have to do with how we canonicalize > > object literal property names? If so a comment would be valuable. > > Going to add a comment: > This is needed for object literal property names. Property names are normalized > to string literals during object literal parsing. Done.
The CQ bit was checked by marja@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2400613003/#ps130001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifier). For this, PreParser needs to accumulate identifiers used in expressions. In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that we get clusterfuzz coverage for it. BUG=chromium:650969, v8:5501 ========== to ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifier). For this, PreParser needs to accumulate identifiers used in expressions. In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that we get clusterfuzz coverage for it. BUG=chromium:650969, v8:5501 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifier). For this, PreParser needs to accumulate identifiers used in expressions. In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that we get clusterfuzz coverage for it. BUG=chromium:650969, v8:5501 ========== to ========== PreParsing inner functions: Fix declaration-only variables, part 2. If an inner function only declares a variable but doesn't use it, Parser and PreParser produced different unresolved variables, and that confused the pessimistic context allocation. This is continuation to https://codereview.chromium.org/2388183003/ This CL fixes more complicated declarations (which are not just one identifier). For this, PreParser needs to accumulate identifiers used in expressions. In addition, this CL manifests FLAG_lazy_inner_functions in tests, so that we get clusterfuzz coverage for it. BUG=chromium:650969, v8:5501 Committed: https://crrev.com/e474e5ffc83cddc0cc1ff7cc9d247f90cd827bfd Cr-Commit-Position: refs/heads/master@{#40112} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e474e5ffc83cddc0cc1ff7cc9d247f90cd827bfd Cr-Commit-Position: refs/heads/master@{#40112} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
