Extract ParseInfo from CompilationInfo.

Extract ParseInfo from CompilationInfo.

Rationale: separate the inputs and outputs of parsing + analysis from the business of compiling (i.e. generating machine code).

BUG=

Committed: https://crrev.com/0f6702562e0ebf40f09fb1d11839f7c9a6d42c39
Cr-Commit-Position: refs/heads/master@{#27078}

[marja, 2015-03-04 09:24:39]

marja@chromium.org changed reviewers:
+ marja@chromium.org

[marja, 2015-03-04 09:24:40]

Nice!

(Tryjobs would be nice.)

Here are some comments (but not an exhaustive list, I'll have a closer look at
some later point):

https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task.cc
File src/background-parsing-task.cc (right):

https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task....
src/background-parsing-task.cc:25:
info->set_source_stream(source->source_stream.get());
Would it make sense to have a ctor of ParseInfo which takes these things as
params?

https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task....
src/background-parsing-task.cc:34: options = ScriptCompiler::kNoCompileOptions;
Huh, the options & options_ modifying is a bit unclear and hard to see that it's
going correctly now.

https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task....
src/background-parsing-task.cc:38: options_ = ScriptCompiler::kNoCompileOptions;
... why is this correct? Won't this just always kill the compile options? (and
we'll never get into the source_->info->set_cached_data(&script_data); branch in
Run).

Some tests *should* fail if you mess this up, though :)

https://codereview.chromium.org/974213002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/974213002/diff/1/src/compiler.cc#newcode171
src/compiler.cc:171: if (parse_info_) parse_info_->DeleteAstValueFactory();
... I guess parse_info_ is not owned, CompilationInfo just happens to point into
it... so it's slightly surprising that the lifetime of CompilationInfo affects
what happens inside ParseInfo.

https://codereview.chromium.org/974213002/diff/1/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/974213002/diff/1/src/compiler.h#newcode381
src/compiler.h:381: ParseInfo* parse_info_;
Owned? Not owned? Who owns it?

https://codereview.chromium.org/974213002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/974213002/diff/1/src/parser.h#newcode27
src/parser.h:27: class ScriptData {
Can you put the added classes (ScriptData and ParseInfo) into a separate file?
parser.h is bloated enough already and diff-wise it's the same.

https://codereview.chromium.org/974213002/diff/1/src/parser.h#newcode228
src/parser.h:228: // TODO(titzer): these should *not* be part of ParseInfo.
Refactor Parser.
Yes! :)

https://codereview.chromium.org/974213002/diff/1/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/974213002/diff/1/src/scopes.cc#newcode12
src/scopes.cc:12: #include "src/scopes.h"
Nit, this should remain at the top...

[rossberg, 2015-03-04 10:25:11]

rossberg@chromium.org changed reviewers:
+ rossberg@chromium.org

[rossberg, 2015-03-04 10:25:12]

Also like it, though I just took a superficial glance. A few suggestions below.

https://codereview.chromium.org/974213002/diff/1/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/974213002/diff/1/src/globals.h#newcode457
src/globals.h:457: enum ParseRestriction {
I don't think this belongs into global.h - it encodes a choice of start symbol
for parsing. In that sense, it has a similar role to flags like
parse_info->is_module(), and should probably be handled analogously. I.e., just
kill the enum and have a is_function flag in parse info. (And later we should
probably unify these flags into one grammar_entry kind of value.)

Short of that, can you at least move it to parser.h?

https://codereview.chromium.org/974213002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/974213002/diff/1/src/parser.h#newcode75
src/parser.h:75: enum Flag {
Can we make the enum private?

https://codereview.chromium.org/974213002/diff/1/src/parser.h#newcode216
src/parser.h:216: //------------- Inputs to parsing and scope analysis
-----------------------
Nit: Would be even more useful to have these sectioning comments on the
accessors in the public part of the class.

[titzer, 2015-03-04 22:39:35]

https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task.cc
File src/background-parsing-task.cc (right):

https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task....
src/background-parsing-task.cc:25:
info->set_source_stream(source->source_stream.get());
On 2015/03/04 09:24:40, marja wrote:
> Would it make sense to have a ctor of ParseInfo which takes these things as
> params?

I tried that, and you end up with ctor hell with default-initializing all the
fields three different times in three different constructors. I changed the
InitFrom* functions to return this, to allow chaining.

https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task....
src/background-parsing-task.cc:34: options = ScriptCompiler::kNoCompileOptions;
On 2015/03/04 09:24:40, marja wrote:
> Huh, the options & options_ modifying is a bit unclear and hard to see that
it's
> going correctly now.

Ok, I've fixed this so the options are only stored in the ParseInfo now.

https://codereview.chromium.org/974213002/diff/1/src/background-parsing-task....
src/background-parsing-task.cc:38: options_ = ScriptCompiler::kNoCompileOptions;
On 2015/03/04 09:24:40, marja wrote:
> ... why is this correct? Won't this just always kill the compile options? (and
> we'll never get into the source_->info->set_cached_data(&script_data); branch
in
> Run).
> 
> Some tests *should* fail if you mess this up, though :)

Fixed.

https://codereview.chromium.org/974213002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/974213002/diff/1/src/compiler.cc#newcode171
src/compiler.cc:171: if (parse_info_) parse_info_->DeleteAstValueFactory();
On 2015/03/04 09:24:40, marja wrote:
> ... I guess parse_info_ is not owned, CompilationInfo just happens to point
into
> it... so it's slightly surprising that the lifetime of CompilationInfo affects
> what happens inside ParseInfo.

Oh yeah, good point, probably a bug. CompilationInfoWithZone owns the ParseInfo
and tears it down in the virtual destructor, but a "naked" CompilationInfo
doesn't own the ParseInfo and should probably not try to mess with its internal
AstValueFactory.

https://codereview.chromium.org/974213002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/974213002/diff/1/src/parser.h#newcode27
src/parser.h:27: class ScriptData {
On 2015/03/04 09:24:40, marja wrote:
> Can you put the added classes (ScriptData and ParseInfo) into a separate file?
> parser.h is bloated enough already and diff-wise it's the same.

ScriptData probably should live somewhere else, but I'm on the fence about
ParseInfo.

https://codereview.chromium.org/974213002/diff/1/src/parser.h#newcode75
src/parser.h:75: enum Flag {
On 2015/03/04 10:25:12, rossberg wrote:
> Can we make the enum private?

Done.

[Michael Starzinger, 2015-03-04 22:50:46]

mstarzinger@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2015-03-04 22:50:46]

https://codereview.chromium.org/974213002/diff/40001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/974213002/diff/40001/src/compiler.cc#newcode495
src/compiler.cc:495: // TODO(titzer): how is that possibly ok?
I can shed light on that. IIRC, the "this_has_uses" flag is set by
HydrogenGraphBuilder and then consumed the the LithiumChunkBuilder, it is merely
initialized here. Another beauty that is the grab-bag of CompilationInfo. In
short, this flag is specific to Crankshaft only.

[marja, 2015-03-05 10:52:55]

moar comments

https://codereview.chromium.org/974213002/diff/80001/src/background-parsing-t...
File src/background-parsing-task.cc (right):

https://codereview.chromium.org/974213002/diff/80001/src/background-parsing-t...
src/background-parsing-task.cc:31: bool disable_lazy =
!Compiler::DebuggerWantsEagerCompilation(isolate);
... shouldn't we disable_lazy exactly when DebuggerWantsEagerCompilation,
without negation?

https://codereview.chromium.org/974213002/diff/80001/src/background-parsing-t...
src/background-parsing-task.cc:39: source->allow_lazy = !disable_lazy;
... and why do we need allow_lazy now that it's already known by info?

https://codereview.chromium.org/974213002/diff/80001/src/background-parsing-t...
File src/background-parsing-task.h (right):

https://codereview.chromium.org/974213002/diff/80001/src/background-parsing-t...
src/background-parsing-task.h:38: SmartPointer<Zone> zone;
Why is Zone needed separately, can't we get it from ParseInfo?

[titzer, 2015-03-05 20:18:31]

https://codereview.chromium.org/974213002/diff/1/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/974213002/diff/1/src/globals.h#newcode457
src/globals.h:457: enum ParseRestriction {
On 2015/03/04 10:25:12, rossberg wrote:
> I don't think this belongs into global.h - it encodes a choice of start symbol
> for parsing. In that sense, it has a similar role to flags like
> parse_info->is_module(), and should probably be handled analogously. I.e.,
just
> kill the enum and have a is_function flag in parse info. (And later we should
> probably unify these flags into one grammar_entry kind of value.)
> 
> Short of that, can you at least move it to parser.h?

Michi argued strongly to keep the enum.

I initially tried to move it to parser.h, but then you end up with a cycle
between compiler.h and parser.h. Actually this cycle already exists and it needs
to be broken carefully to reduce the over-inclusion of compiler.h. Couldn't
figure out how to do that yet.

https://codereview.chromium.org/974213002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/974213002/diff/1/src/parser.h#newcode27
src/parser.h:27: class ScriptData {
On 2015/03/04 22:39:35, titzer wrote:
> On 2015/03/04 09:24:40, marja wrote:
> > Can you put the added classes (ScriptData and ParseInfo) into a separate
file?
> > parser.h is bloated enough already and diff-wise it's the same.
> 
> ScriptData probably should live somewhere else, but I'm on the fence about
> ParseInfo.

I've moved ScriptData to preparse-data.h

https://codereview.chromium.org/974213002/diff/1/src/parser.h#newcode228
src/parser.h:228: // TODO(titzer): these should *not* be part of ParseInfo.
Refactor Parser.
On 2015/03/04 09:24:40, marja wrote:
> Yes! :)

I'm not sure exactly where these will land, but I was thinking of further
splitting out another small struct with just these handles.

https://codereview.chromium.org/974213002/diff/80001/src/background-parsing-t...
File src/background-parsing-task.cc (right):

https://codereview.chromium.org/974213002/diff/80001/src/background-parsing-t...
src/background-parsing-task.cc:31: bool disable_lazy =
!Compiler::DebuggerWantsEagerCompilation(isolate);
On 2015/03/05 10:52:55, marja wrote:
> ... shouldn't we disable_lazy exactly when DebuggerWantsEagerCompilation,
> without negation?

Good catch, thankfully the test failed :-)

https://codereview.chromium.org/974213002/diff/80001/src/background-parsing-t...
src/background-parsing-task.cc:39: source->allow_lazy = !disable_lazy;
On 2015/03/05 10:52:55, marja wrote:
> ... and why do we need allow_lazy now that it's already known by info?

Good point, I've removed both allow_lazy and hash_seed.

Also, I've removed the boolean allow_lazy from Parser::ParseStatic, since it is
now contained in the ParseInfo.

[Michael Starzinger, 2015-03-05 20:56:55]

https://codereview.chromium.org/974213002/diff/1/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/974213002/diff/1/src/globals.h#newcode457
src/globals.h:457: enum ParseRestriction {
On 2015/03/05 20:18:30, titzer wrote:
> On 2015/03/04 10:25:12, rossberg wrote:
> > I don't think this belongs into global.h - it encodes a choice of start
symbol
> > for parsing. In that sense, it has a similar role to flags like
> > parse_info->is_module(), and should probably be handled analogously. I.e.,
> just
> > kill the enum and have a is_function flag in parse info. (And later we
should
> > probably unify these flags into one grammar_entry kind of value.)
> > 
> > Short of that, can you at least move it to parser.h?
> 
> Michi argued strongly to keep the enum.
> 
> I initially tried to move it to parser.h, but then you end up with a cycle
> between compiler.h and parser.h. Actually this cycle already exists and it
needs
> to be broken carefully to reduce the over-inclusion of compiler.h. Couldn't
> figure out how to do that yet.

I argued strongly for "not turning it into a boolean", correct. I would be fine
with the parser just reporting whether it consumed exactly one single function
literal or not, and then the caller checking that result. That would be fine
with me. FYI, the caller is all the way up in Runtime_CompileString.

[rossberg, 2015-03-06 07:03:40]

On 2015/03/05 20:56:55, Michael Starzinger wrote:
> https://codereview.chromium.org/974213002/diff/1/src/globals.h
> File src/globals.h (right):
> 
> https://codereview.chromium.org/974213002/diff/1/src/globals.h#newcode457
> src/globals.h:457: enum ParseRestriction {
> On 2015/03/05 20:18:30, titzer wrote:
> > On 2015/03/04 10:25:12, rossberg wrote:
> > > I don't think this belongs into global.h - it encodes a choice of start
> symbol
> > > for parsing. In that sense, it has a similar role to flags like
> > > parse_info->is_module(), and should probably be handled analogously. I.e.,
> > just
> > > kill the enum and have a is_function flag in parse info. (And later we
> should
> > > probably unify these flags into one grammar_entry kind of value.)
> > > 
> > > Short of that, can you at least move it to parser.h?
> > 
> > Michi argued strongly to keep the enum.
> > 
> > I initially tried to move it to parser.h, but then you end up with a cycle
> > between compiler.h and parser.h. Actually this cycle already exists and it
> needs
> > to be broken carefully to reduce the over-inclusion of compiler.h. Couldn't
> > figure out how to do that yet.
> 
> I argued strongly for "not turning it into a boolean", correct. I would be
fine
> with the parser just reporting whether it consumed exactly one single function
> literal or not, and then the caller checking that result. That would be fine
> with me. FYI, the caller is all the way up in Runtime_CompileString.

Hm, that would seem backwards to me. This is really about what grammar start
symbol to enter parsing with.

For modules we have the same problem, and the caller does set_is_module on the
parseinfo. Would you be opposed to have an analogous is_function to set?
(Ideally, one combined enum instead of separate flags, but that'd be for another
clean-up.)

[Michael Starzinger, 2015-03-06 08:36:28]

On 2015/03/06 07:03:40, rossberg wrote:
> On 2015/03/05 20:56:55, Michael Starzinger wrote:
> > https://codereview.chromium.org/974213002/diff/1/src/globals.h
> > File src/globals.h (right):
> > 
> > https://codereview.chromium.org/974213002/diff/1/src/globals.h#newcode457
> > src/globals.h:457: enum ParseRestriction {
> > On 2015/03/05 20:18:30, titzer wrote:
> > > On 2015/03/04 10:25:12, rossberg wrote:
> > > > I don't think this belongs into global.h - it encodes a choice of start
> > symbol
> > > > for parsing. In that sense, it has a similar role to flags like
> > > > parse_info->is_module(), and should probably be handled analogously.
I.e.,
> > > just
> > > > kill the enum and have a is_function flag in parse info. (And later we
> > should
> > > > probably unify these flags into one grammar_entry kind of value.)
> > > > 
> > > > Short of that, can you at least move it to parser.h?
> > > 
> > > Michi argued strongly to keep the enum.
> > > 
> > > I initially tried to move it to parser.h, but then you end up with a cycle
> > > between compiler.h and parser.h. Actually this cycle already exists and it
> > needs
> > > to be broken carefully to reduce the over-inclusion of compiler.h.
Couldn't
> > > figure out how to do that yet.
> > 
> > I argued strongly for "not turning it into a boolean", correct. I would be
> fine
> > with the parser just reporting whether it consumed exactly one single
function
> > literal or not, and then the caller checking that result. That would be fine
> > with me. FYI, the caller is all the way up in Runtime_CompileString.
> 
> Hm, that would seem backwards to me. This is really about what grammar start
> symbol to enter parsing with.
> 
> For modules we have the same problem, and the caller does set_is_module on the
> parseinfo. Would you be opposed to have an analogous is_function to set?
> (Ideally, one combined enum instead of separate flags, but that'd be for
another
> clean-up.)

I am fine with that (both a separate is_function flag, or a combined enum) as
long as it doesn't lead to a random stray boolean in the signature of
Compiler::GetFunctionFromEval.

[marja, 2015-03-06 08:40:40]

... you could avoid the cyclic dependency by putting ParseInfo and related
classes into a separate file, no?

(It would also make sense; if somebody wants to use the end result of parsing,
which is in ParseInfo, no need to include the Parser itself!)

[marja, 2015-03-09 09:18:07]

I had a look at the following parts: parser, compiler, scopes,
background-parsing-task, tests, parser-shell. They lgtm modulo the comments
(afaics it's leaking Zone). (The other parts look straightforward too, can't see
anything wrong there either, you can decide whether they're non-trivial enough
to ask somebody else to review too.)

https://codereview.chromium.org/974213002/diff/120001/src/background-parsing-...
File src/background-parsing-task.cc (left):

https://codereview.chromium.org/974213002/diff/120001/src/background-parsing-...
src/background-parsing-task.cc:20: // We don't set the context to the
CompilationInfo yet, because the background
This comment is probably still relevant; why was it removed?

https://codereview.chromium.org/974213002/diff/120001/src/background-parsing-...
File src/background-parsing-task.cc (right):

https://codereview.chromium.org/974213002/diff/120001/src/background-parsing-...
src/background-parsing-task.cc:20: Zone* zone = new Zone();
Who deletes Zone? ParseInfo doesn't own it afaics.

https://codereview.chromium.org/974213002/diff/120001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/974213002/diff/120001/src/compiler.cc#newcode...
src/compiler.cc:1099: info->parse_info()->set_cached_data(nullptr);
Nit: you already have parse_info, so s/info->parse_info()/parse_info/

https://codereview.chromium.org/974213002/diff/120001/src/compiler.cc#newcode...
src/compiler.cc:1593: if (parse_info_) {
Nit: unnecessary if, since delete 0 is ok.

[titzer, 2015-03-09 13:03:09]

https://codereview.chromium.org/974213002/diff/120001/src/background-parsing-...
File src/background-parsing-task.cc (left):

https://codereview.chromium.org/974213002/diff/120001/src/background-parsing-...
src/background-parsing-task.cc:20: // We don't set the context to the
CompilationInfo yet, because the background
On 2015/03/09 09:18:07, marja wrote:
> This comment is probably still relevant; why was it removed?

Done.

https://codereview.chromium.org/974213002/diff/120001/src/background-parsing-...
File src/background-parsing-task.cc (right):

https://codereview.chromium.org/974213002/diff/120001/src/background-parsing-...
src/background-parsing-task.cc:20: Zone* zone = new Zone();
On 2015/03/09 09:18:07, marja wrote:
> Who deletes Zone? ParseInfo doesn't own it afaics.

I've changed it back to a SmartPointer in BackgroundParsingTask, so it will be
deleted when the BackgroundParsingTask is.

https://codereview.chromium.org/974213002/diff/120001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/974213002/diff/120001/src/compiler.cc#newcode...
src/compiler.cc:1099: info->parse_info()->set_cached_data(nullptr);
On 2015/03/09 09:18:07, marja wrote:
> Nit: you already have parse_info, so s/info->parse_info()/parse_info/

Done.

https://codereview.chromium.org/974213002/diff/120001/src/compiler.cc#newcode...
src/compiler.cc:1593: if (parse_info_) {
On 2015/03/09 09:18:07, marja wrote:
> Nit: unnecessary if, since delete 0 is ok.

Done.

[titzer, 2015-03-09 14:22:05]

The CQ bit was checked by titzer@chromium.org

[titzer, 2015-03-09 14:22:06]

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/974213002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-03-09 14:23:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/974213002/220001

[commit-bot: I haz the power, 2015-03-09 14:51:17]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-03-09 14:51:38]

Patchset 12 (id:??) landed as
https://crrev.com/0f6702562e0ebf40f09fb1d11839f7c9a6d42c39
Cr-Commit-Position: refs/heads/master@{#27078}