Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2306413002:
Fully deserialize the scope chain after parsing, not before (Closed)
|
Created:
4 years, 3 months ago by jochen (gone - plz use gerrit) Modified:
4 years, 3 months ago CC:
Michael Starzinger, rmcilroy, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionFully deserialize the scope chain after parsing, not before
To avoid a dependency on the heap during parsing, we only create a scope chain
without linking to the associated ScopeInfo objects before parsing. This is
enough to avoid special cases during parsing of arrow functions / eval.
Looking at the outer scope's variables during parsing was only needed for hosting
sloppy block functions inside eval. To be able to do this now, we hoist for the
outer-most eval scope after parsing, in DeclarationScope::Analyze.
DeclarationScope::Analyze is also where we replace the outer scope chain with the
fully deserialized version, so variables can be resolved.
Also, this unifies background and foreground thread parsing, as we don't have to
worry about ScopeInfos getting accessed before we're back on the main thread.
BUG=v8:5215
R=verwaest@chromium.org,marja@chromium.org,adamk@chromium.org
Committed: https://crrev.com/94492437d9696f70f5a3ce0b3dc91026cc021abe
Cr-Commit-Position: refs/heads/master@{#39452}
Patch Set 1 #Patch Set 2 : updates #Patch Set 3 : updates #Patch Set 4 : updates #
Total comments: 18
Patch Set 5 : updates #Patch Set 6 : updates #
Total comments: 14
Patch Set 7 : updates #Patch Set 8 : new approach #Patch Set 9 : cleanup #
Total comments: 4
Patch Set 10 : updates #
Dependent Patchsets: Messages
Total messages: 66 (41 generated)
The CQ bit was checked by jochen@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...)
Description was changed from ========== Deserialize the scope chain after parsing, not before We only look at the outer scopes for resolving variables, so there's no need to do this before. Also, this unifies background and foreground thread parsing, as we don't have to worry about ScopeInfos getting accessed before we're back on the main thread. BUG=v8:5215 R=verwaest@chromium.org,marja@chromium.org,adamk@chromium.org ========== to ========== Deserialize the scope chain after parsing, not before We only look at the outer scopes for resolving variables, so there's no need to do this before. Also, this unifies background and foreground thread parsing, as we don't have to worry about ScopeInfos getting accessed before we're back on the main thread. BUG=v8:5215 R=verwaest@chromium.org,marja@chromium.org,adamk@chromium.org ==========
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/12441)
The CQ bit was checked by jochen@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...
ptal there's a new function InspectScopeChain that looks at the declaration scope and stores what type it is in the parser-base, but doesn't deserialize the scope chain yet. That's only a problem if we'd hoist a function declaration out of an eval call, so stop doing that during parsing, but do it after parsing, when we deserialize the scope chain as part of Scope::Analyze. I also removed the code to deserialize the scope chain off heap, as this is no longer needed. In a follow-up, I'll remove ParseInfo::context() and replace it with ParseInfo::scope_info()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
High-level note: more of your message belongs in the CL description (particularly the stuff about sloppy eval), since there's definitely a tradeoff here: constructing AST in Scope after parsing is done is a very strange thing to be doing (of course, so are the semantics of eval hoisting out of blocks).
Description was changed from ========== Deserialize the scope chain after parsing, not before We only look at the outer scopes for resolving variables, so there's no need to do this before. Also, this unifies background and foreground thread parsing, as we don't have to worry about ScopeInfos getting accessed before we're back on the main thread. BUG=v8:5215 R=verwaest@chromium.org,marja@chromium.org,adamk@chromium.org ========== to ========== Deserialize the scope chain after parsing, not before We only look at the outer scopes for resolving variables, so there's no need to do this before. There's a new function InspectScopeChain that looks at the declaration scope and stores what type it is in the parser-base, but doesn't deserialize the scope chain yet. That's also a problem if we'd hoist a function declaration out of an eval call, so we stop doing that during parsing, but do it after parsing, when we deserialize the scope chain later as part of Scope::Analyze. I also removed the code to deserialize the scope chain off heap, as this is no longer needed. In a follow-up, I'll remove ParseInfo::context() and replace it with ParseInfo::scope_info() Also, this unifies background and foreground thread parsing, as we don't have to worry about ScopeInfos getting accessed before we're back on the main thread. BUG=v8:5215 R=verwaest@chromium.org,marja@chromium.org,adamk@chromium.org ==========
done
Idk the right answer to "how to make Scope not create AST nodes" problems yet :( https://codereview.chromium.org/2306413002/diff/60001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2306413002/diff/60001/src/ast/scopes.cc#newco... src/ast/scopes.cc:438: if (is_eval_scope() && !outer_scope()->outer_scope()) return; Style nit: outer_scope()->outer_scope() == nullptr https://codereview.chromium.org/2306413002/diff/60001/src/ast/scopes.cc#newco... src/ast/scopes.cc:438: if (is_eval_scope() && !outer_scope()->outer_scope()) return; Add a comment which explains that hoisting for eval scopes is done later + why. https://codereview.chromium.org/2306413002/diff/60001/src/ast/scopes.cc#newco... src/ast/scopes.cc:526: // Create VariableProxies for creating an assignment statement This is the workaround for not being able to create Assignments! So you need one or the other, but not both. https://codereview.chromium.org/2306413002/diff/60001/src/ast/scopes.cc#newco... src/ast/scopes.cc:532: Expression* assignment = factory->NewAssignment( Hmm, I wrote the exact same code and people had objections, and I don't think that has changed... (Not sure what would be an alternative solution...) https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:873: if (receiver_scope->outer_scope()) return receiver_scope->function_kind(); Style nit: != nullptr (in several places) https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:878: if (receiver_scope->outer_scope()) { Why is this correct? If there's no outer scope, then we just don't record anything anywhere...? (The answer can be a comment:) https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:879: return receiver_scope->RecordSuperPropertyUsage(); Nit: "return" is unnecessary here. https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:1355: FunctionKind outer_most_receivers_function_kind_; Naming nit: "outermost" is one word, so outermost_receiver_scope_function_kind_ etc. (Or why "receivers"?) https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:610: info->set_allow_lazy_parsing(false); ... here I was wondering why we do this... there's one comment that for strict evals we eager parse, because they cannot pollute the outer name space anyway, so they are assumed not to contain code which is not immediately executed. But why for normal evals? Because also there we assume that there's no code which would benefit from lazy parsing? Anyhow, not a problem of this CL. https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser.h#ne... src/parsing/parser.h:185: void InspectScopeChain(ParseInfo* info, Handle<ScopeInfo> scope_info); Pls add a comment explaining what this does.
https://codereview.chromium.org/2306413002/diff/60001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2306413002/diff/60001/src/ast/scopes.cc#newco... src/ast/scopes.cc:438: if (is_eval_scope() && !outer_scope()->outer_scope()) return; On 2016/09/12 at 07:50:32, marja wrote: > Add a comment which explains that hoisting for eval scopes is done later + why. done > Style nit: outer_scope()->outer_scope() == nullptr I think that's unnecessarily verbose (read: neither in the style guide, nor used all the time) https://codereview.chromium.org/2306413002/diff/60001/src/ast/scopes.cc#newco... src/ast/scopes.cc:532: Expression* assignment = factory->NewAssignment( On 2016/09/12 at 07:50:32, marja wrote: > Hmm, I wrote the exact same code and people had objections, and I don't think that has changed... > > (Not sure what would be an alternative solution...) AST rewriting happens around the same time. The main problem is that I first need to deserialize the scope chain, the hoist the functions, then resolve the variables. I guess I could introduce an explicit call to deserialize scope chain that has to happen before ast rewriting https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:878: if (receiver_scope->outer_scope()) { On 2016/09/12 at 07:50:33, marja wrote: > Why is this correct? If there's no outer scope, then we just don't record anything anywhere...? (The answer can be a comment:) done https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:879: return receiver_scope->RecordSuperPropertyUsage(); On 2016/09/12 at 07:50:32, marja wrote: > Nit: "return" is unnecessary here. done https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:1355: FunctionKind outer_most_receivers_function_kind_; On 2016/09/12 at 07:50:32, marja wrote: > Naming nit: "outermost" is one word, so outermost_receiver_scope_function_kind_ etc. > > (Or why "receivers"?) done https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:610: info->set_allow_lazy_parsing(false); On 2016/09/12 at 07:50:33, marja wrote: > ... here I was wondering why we do this... there's one comment that for strict evals we eager parse, because they cannot pollute the outer name space anyway, so they are assumed not to contain code which is not immediately executed. But why for normal evals? Because also there we assume that there's no code which would benefit from lazy parsing? > > Anyhow, not a problem of this CL. I moved this from line 722 in the old code https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2306413002/diff/60001/src/parsing/parser.h#ne... src/parsing/parser.h:185: void InspectScopeChain(ParseInfo* info, Handle<ScopeInfo> scope_info); On 2016/09/12 at 07:50:33, marja wrote: > Pls add a comment explaining what this does. done
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jochen@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...
https://codereview.chromium.org/2306413002/diff/60001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2306413002/diff/60001/src/ast/scopes.cc#newco... src/ast/scopes.cc:526: // Create VariableProxies for creating an assignment statement On 2016/09/12 at 07:50:32, marja wrote: > This is the workaround for not being able to create Assignments! So you need one or the other, but not both. done
lgtm Offline discussion: We don't see a non-ugly way to make Scope not create Statements :( It would be possible to always create the Statement and store it in the SloppyBlockFunctionStatement in such a way that it's not returned by statement(), and only take it into use (i.e., set it so that it's returned by statement()) later. But it's ugly. Or is it. I can't decide. If it's not, it can be done as a follow-up. Re: if (somepointer) vs. if (somepointer != nullptr); the latter seems prevalent in scopes.cc.
On 2016/09/12 at 08:51:25, marja wrote: > lgtm > > Offline discussion: We don't see a non-ugly way to make Scope not create Statements :( > > It would be possible to always create the Statement and store it in the SloppyBlockFunctionStatement in such a way that it's not returned by statement(), and only take it into use (i.e., set it so that it's returned by statement()) later. But it's ugly. Or is it. I can't decide. If it's not, it can be done as a follow-up. although that might create many more statements than what we currently do... > > Re: if (somepointer) vs. if (somepointer != nullptr); the latter seems prevalent in scopes.cc.
Adam, do you have further comments?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I would like to take one more pass on this today (which is full of meetings :( )
The CL description reads as a list of things this CL does, but it's hard for me to tell which of those things are essential to the project of issue 5215 and which are just related work. In particular, I don't think I fully understand what problem is solved by moving the deserialization of the scope chain into Scope::Analyze. Is the final paragraph the answer to that question? "Also, this unifies background and foreground thread parsing, as we don't have to worry about ScopeInfos getting accessed before we're back on the main thread." As noted below, delaying scope chain creation adds more special-casing to the Parser's handling of scopes, where previously logic in the main bits of the parser could assume that the scope chain available was the same whether working on a "real" scope chain or on a deserialized one, and I worry that this might be a vector for parser bugs to creep in over time. https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc#newc... src/ast/scopes.cc:318: if (!info->context().is_null() && !info->context()->IsNativeContext()) { Can you make this an early return instead? I'd find that easier to parse. https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc#newc... src/ast/scopes.cc:319: ReplaceOuterScope(Scope::DeserializeScopeChain( Nit: You shouldn't need to qualify DeserializeScopeChain here. https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc#newc... src/ast/scopes.cc:437: bool* ok) { It doesn't look like this out parameter should ever get set to false, so I think it should go away. https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc#newc... src/ast/scopes.cc:525: DCHECK(*ok); // Based on the preceding check, this should not fail Let's make this a CHECK instead and remove the out parameter. https://codereview.chromium.org/2306413002/diff/100001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2306413002/diff/100001/src/parsing/parser-bas... src/parsing/parser-base.h:878: // If the receiver scope is the outermost script scope, a) it might not be This comment points to some weirdness in this CL. The parser now has to think about two different states, one where it has outer scopes and one where it doesn't. And for every bit of outer-scope handling it does, some consideration must be made as to what the right handling is. This means that code which naively does something with scope()->GetReceiverScope() is now wrong. https://codereview.chromium.org/2306413002/diff/100001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2306413002/diff/100001/src/parsing/parser.h#n... src/parsing/parser.h:192: void InspectScopeChain(ParseInfo* info, MaybeHandle<ScopeInfo> scope_info); "Inspect" in this function name isn't helpful at all to understanding what it does (also, "inspect" is normally an idempotent operation). This makes it hard to know what bits it's going to set, and what logic belongs (or doesn't belong) inside it. In fact, it already seems to do two different things: one is setting up ParseInfo::script_scope(), and the other is setting this ReceiverScope info. Some names I don't love but like more than InspectScopeChain: - InitializeScriptAndReceiverScopeInfo - InitializeOuterScopeData - InitiatizeScopeChain Also please rename "scope_info" to "outer_scope_info"
yeah, so the main motivation is to not touch the heap during parsing. There are no two modes, however, we'll just never have the "correct" outer scope chain during parsing.. https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc#newc... src/ast/scopes.cc:318: if (!info->context().is_null() && !info->context()->IsNativeContext()) { On 2016/09/12 at 16:57:47, adamk wrote: > Can you make this an early return instead? I'd find that easier to parse. done https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc#newc... src/ast/scopes.cc:319: ReplaceOuterScope(Scope::DeserializeScopeChain( On 2016/09/12 at 16:57:47, adamk wrote: > Nit: You shouldn't need to qualify DeserializeScopeChain here. done https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc#newc... src/ast/scopes.cc:437: bool* ok) { On 2016/09/12 at 16:57:47, adamk wrote: > It doesn't look like this out parameter should ever get set to false, so I think it should go away. done https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc#newc... src/ast/scopes.cc:525: DCHECK(*ok); // Based on the preceding check, this should not fail On 2016/09/12 at 16:57:47, adamk wrote: > Let's make this a CHECK instead and remove the out parameter. done https://codereview.chromium.org/2306413002/diff/100001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2306413002/diff/100001/src/parsing/parser-bas... src/parsing/parser-base.h:878: // If the receiver scope is the outermost script scope, a) it might not be On 2016/09/12 at 16:57:47, adamk wrote: > This comment points to some weirdness in this CL. The parser now has to think about two different states, one where it has outer scopes and one where it doesn't. And for every bit of outer-scope handling it does, some consideration must be made as to what the right handling is. > > This means that code which naively does something with scope()->GetReceiverScope() is now wrong. it's a bit easier - the parser just never has an correct outer scope. Of course, this still means that using GetReceiverScope() will result in an incorrect scope if we parse an arrow function or eval. https://codereview.chromium.org/2306413002/diff/100001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2306413002/diff/100001/src/parsing/parser.h#n... src/parsing/parser.h:192: void InspectScopeChain(ParseInfo* info, MaybeHandle<ScopeInfo> scope_info); On 2016/09/12 at 16:57:47, adamk wrote: > "Inspect" in this function name isn't helpful at all to understanding what it does (also, "inspect" is normally an idempotent operation). This makes it hard to know what bits it's going to set, and what logic belongs (or doesn't belong) inside it. In fact, it already seems to do two different things: one is setting up ParseInfo::script_scope(), and the other is setting this ReceiverScope info. > > Some names I don't love but like more than InspectScopeChain: > > - InitializeScriptAndReceiverScopeInfo > - InitializeOuterScopeData > - InitiatizeScopeChain > > Also please rename "scope_info" to "outer_scope_info" done
https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2306413002/diff/100001/src/ast/scopes.cc#newc... src/ast/scopes.cc:319: ReplaceOuterScope(Scope::DeserializeScopeChain( On 2016/09/12 at 19:00:43, jochen wrote: > On 2016/09/12 at 16:57:47, adamk wrote: > > Nit: You shouldn't need to qualify DeserializeScopeChain here. > > done Actually, the compiler disagrees so I reverted this back
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2306413002/diff/100001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2306413002/diff/100001/src/parsing/parser-bas... src/parsing/parser-base.h:878: // If the receiver scope is the outermost script scope, a) it might not be On 2016/09/12 19:00:43, jochen wrote: > On 2016/09/12 at 16:57:47, adamk wrote: > > This comment points to some weirdness in this CL. The parser now has to think > about two different states, one where it has outer scopes and one where it > doesn't. And for every bit of outer-scope handling it does, some consideration > must be made as to what the right handling is. > > > > This means that code which naively does something with > scope()->GetReceiverScope() is now wrong. > > it's a bit easier - the parser just never has an correct outer scope. Of course, > this still means that using GetReceiverScope() will result in an incorrect scope > if we parse an arrow function or eval. The two modes I'm talking about are: - Deep inside some inner scope, in which case the "outer scope chain" _is_ available (since we're in the middle of parsing it) - Near the outer scope, in which case we have to have special logic Previously if I was parsing an expression I didn't have to care where in the scope chain I was; no I do.
understood (however, I think the differences aren't that bad in practice, as this only matters if the top most scope we parse is either a sloppy eval, or an arrow function). An alternative approach would be to deserialize the scope chain before parsing, but don't reference the scope infos, so it's possible to always get the right receiver scope, but it won't be possible to look at variables (so we'd still need to move the hoisting as this CL does). wdyt?
On 2016/09/13 07:11:13, jochen wrote: > understood (however, I think the differences aren't that bad in practice, as > this only matters if the top most scope we parse is either a sloppy eval, or an > arrow function). The rarity of the special case doesn't matter so much as its existence, imho. > An alternative approach would be to deserialize the scope chain before parsing, > but don't reference the scope infos, so it's possible to always get the right > receiver scope, but it won't be possible to look at variables (so we'd still > need to move the hoisting as this CL does). > > wdyt? That sounds much better to me, and suggests a reasonable guideline for how the parser should think about outer scopes, which is not to worry about what variables are there (which is already necessary in all cases _except_ the Annex B hoisting since you never know if your outer scope is complete yet).
The CQ bit was checked by jochen@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...
updated, ptal I'll rework the CL description and get another review from Marja if this now looks good enough to you :)
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 jochen@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 ========== Deserialize the scope chain after parsing, not before We only look at the outer scopes for resolving variables, so there's no need to do this before. There's a new function InspectScopeChain that looks at the declaration scope and stores what type it is in the parser-base, but doesn't deserialize the scope chain yet. That's also a problem if we'd hoist a function declaration out of an eval call, so we stop doing that during parsing, but do it after parsing, when we deserialize the scope chain later as part of Scope::Analyze. I also removed the code to deserialize the scope chain off heap, as this is no longer needed. In a follow-up, I'll remove ParseInfo::context() and replace it with ParseInfo::scope_info() Also, this unifies background and foreground thread parsing, as we don't have to worry about ScopeInfos getting accessed before we're back on the main thread. BUG=v8:5215 R=verwaest@chromium.org,marja@chromium.org,adamk@chromium.org ========== to ========== Fully deserialize the scope chain after parsing, not before To avoid a dependency on the heap during parsing, we only create a scope chain without linking to the associated ScopeInfo objects before parsing. This is enough to avoid special cases during parsing of arrow functions / eval. We don't have to look at the outer scope's variables during parsing with one exception: when parsing eval(), we might hoist a function depending on whether a let declaration with the same name exists in an outer scope or not. To be able to do this, we only hoist for the outer-most eval scope after parsing, in DeclarationScope::Analyze. DeclarationScope::Analyze is also where we replace the outer scope chain with the fully deserialized version, so variables can be resolved. Also, this unifies background and foreground thread parsing, as we don't have to worry about ScopeInfos getting accessed before we're back on the main thread. BUG=v8:5215 R=verwaest@chromium.org,marja@chromium.org,adamk@chromium.org ==========
cleaned up the patch a bit to minimize the diff, and updated CL description. ptal!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ comments https://codereview.chromium.org/2306413002/diff/160001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2306413002/diff/160001/src/ast/scopes.cc#newc... src/ast/scopes.cc:535: scope->ReplaceOuterScope(Scope::DeserializeScopeChain( (I was slightly surprised that we construct new Scope objects here instead of just adding ScopeInfos to existing objects, but I guess this is simpler to implement.) https://codereview.chromium.org/2306413002/diff/160001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2306413002/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:3899: // actual scope chain is available. Nit: this comment still doesn't explain why. Also, is the outermost eval scope necessarily inside a script scope? What if we're just parsing an eval which is inside a function scope? That's confusing. Maybe something like this: For the outermost eval scope, we cannot hoist during parsing: let declarations in the surrounding scope may prevent hoisting, but the information is unaccessible during parsing. In this case, we hoist later in DeclarationScope::Analyze.
Description nit: "We don't have to look at the outer scope's variables during parsing with one exception: when parsing eval(), ..." This reads as "we don't look at the variables except when we do", you probably wanted to emphasize *have to*, but it's not clear to the reader. Maybe replace with "looking at outer scope's variables was only needed for hoisting sloppy block functions inside eval + <explanation how that is solved now>" or sth.
Description was changed from ========== Fully deserialize the scope chain after parsing, not before To avoid a dependency on the heap during parsing, we only create a scope chain without linking to the associated ScopeInfo objects before parsing. This is enough to avoid special cases during parsing of arrow functions / eval. We don't have to look at the outer scope's variables during parsing with one exception: when parsing eval(), we might hoist a function depending on whether a let declaration with the same name exists in an outer scope or not. To be able to do this, we only hoist for the outer-most eval scope after parsing, in DeclarationScope::Analyze. DeclarationScope::Analyze is also where we replace the outer scope chain with the fully deserialized version, so variables can be resolved. Also, this unifies background and foreground thread parsing, as we don't have to worry about ScopeInfos getting accessed before we're back on the main thread. BUG=v8:5215 R=verwaest@chromium.org,marja@chromium.org,adamk@chromium.org ========== to ========== Fully deserialize the scope chain after parsing, not before To avoid a dependency on the heap during parsing, we only create a scope chain without linking to the associated ScopeInfo objects before parsing. This is enough to avoid special cases during parsing of arrow functions / eval. Looking at the outer scope's variables during parsing was only needed for hosting sloppy block functions inside eval. To be able to do this now, we hoist for the outer-most eval scope after parsing, in DeclarationScope::Analyze. DeclarationScope::Analyze is also where we replace the outer scope chain with the fully deserialized version, so variables can be resolved. Also, this unifies background and foreground thread parsing, as we don't have to worry about ScopeInfos getting accessed before we're back on the main thread. BUG=v8:5215 R=verwaest@chromium.org,marja@chromium.org,adamk@chromium.org ==========
updated https://codereview.chromium.org/2306413002/diff/160001/src/ast/scopes.cc File src/ast/scopes.cc (right): https://codereview.chromium.org/2306413002/diff/160001/src/ast/scopes.cc#newc... src/ast/scopes.cc:535: scope->ReplaceOuterScope(Scope::DeserializeScopeChain( On 2016/09/14 at 09:45:21, marja wrote: > (I was slightly surprised that we construct new Scope objects here instead of just adding ScopeInfos to existing objects, but I guess this is simpler to implement.) yeah.. https://codereview.chromium.org/2306413002/diff/160001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2306413002/diff/160001/src/parsing/parser.cc#... src/parsing/parser.cc:3899: // actual scope chain is available. On 2016/09/14 at 09:45:21, marja wrote: > Nit: this comment still doesn't explain why. Also, is the outermost eval scope necessarily inside a script scope? What if we're just parsing an eval which is inside a function scope? That's confusing. > > Maybe something like this: > > For the outermost eval scope, we cannot hoist during parsing: let declarations in the surrounding scope may prevent hoisting, but the information is unaccessible during parsing. In this case, we hoist later in DeclarationScope::Analyze. done
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by jochen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org Link to the patchset: https://codereview.chromium.org/2306413002/#ps180001 (title: "updates")
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.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Fully deserialize the scope chain after parsing, not before To avoid a dependency on the heap during parsing, we only create a scope chain without linking to the associated ScopeInfo objects before parsing. This is enough to avoid special cases during parsing of arrow functions / eval. Looking at the outer scope's variables during parsing was only needed for hosting sloppy block functions inside eval. To be able to do this now, we hoist for the outer-most eval scope after parsing, in DeclarationScope::Analyze. DeclarationScope::Analyze is also where we replace the outer scope chain with the fully deserialized version, so variables can be resolved. Also, this unifies background and foreground thread parsing, as we don't have to worry about ScopeInfos getting accessed before we're back on the main thread. BUG=v8:5215 R=verwaest@chromium.org,marja@chromium.org,adamk@chromium.org ========== to ========== Fully deserialize the scope chain after parsing, not before To avoid a dependency on the heap during parsing, we only create a scope chain without linking to the associated ScopeInfo objects before parsing. This is enough to avoid special cases during parsing of arrow functions / eval. Looking at the outer scope's variables during parsing was only needed for hosting sloppy block functions inside eval. To be able to do this now, we hoist for the outer-most eval scope after parsing, in DeclarationScope::Analyze. DeclarationScope::Analyze is also where we replace the outer scope chain with the fully deserialized version, so variables can be resolved. Also, this unifies background and foreground thread parsing, as we don't have to worry about ScopeInfos getting accessed before we're back on the main thread. BUG=v8:5215 R=verwaest@chromium.org,marja@chromium.org,adamk@chromium.org Committed: https://crrev.com/94492437d9696f70f5a3ce0b3dc91026cc021abe Cr-Commit-Position: refs/heads/master@{#39452} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/94492437d9696f70f5a3ce0b3dc91026cc021abe Cr-Commit-Position: refs/heads/master@{#39452} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
