Create multiple compilation jobs for ignition if compiling multiple literals

Create multiple compilation jobs for ignition if compiling multiple literals

BUG=v8:5394
R=mstarzinger@chromium.org,rmcilroy@chromium.org

[jochen (gone - plz use gerrit), 2016-10-07 09:30:41]

fyi, still doesn't pass tests, but this is what I plan to do roughly

[rmcilroy, 2016-10-07 14:26:35]

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

https://codereview.chromium.org/2399463008/diff/1/src/compiler.h#newcode60
src/compiler.h:60: PrepareUnoptimizedCompilationJob(CompilationInfo* info);
nit - Job->Jobs

https://codereview.chromium.org/2399463008/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2399463008/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:238: : public
AstTraversalVisitor<CollectEagerFunctionLiterals> {
I'd really prefer we do this somewhere else - we already walk the AST too many
times IMO. Can't we do this in ast-renumbering or something?

https://codereview.chromium.org/2399463008/diff/1/src/interpreter/interpreter.h
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/2399463008/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.h:48: static
std::vector<std::unique_ptr<CompilationJob>> NewCompilationJob(
nit - Jobs and update the comment.

[jochen (gone - plz use gerrit), 2016-10-07 14:55:25]

https://codereview.chromium.org/2399463008/diff/1/src/interpreter/interpreter.cc
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2399463008/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.cc:238: : public
AstTraversalVisitor<CollectEagerFunctionLiterals> {
On 2016/10/07 at 14:26:34, rmcilroy wrote:
> I'd really prefer we do this somewhere else - we already walk the AST too many
times IMO. Can't we do this in ast-renumbering or something?

none of the other visitors visit the entire AST (other than the bytecode
generator of course) ...

[rmcilroy, 2016-10-07 15:20:34]

On 2016/10/07 14:55:25, jochen wrote:
>
https://codereview.chromium.org/2399463008/diff/1/src/interpreter/interpreter.cc
> File src/interpreter/interpreter.cc (right):
> 
>
https://codereview.chromium.org/2399463008/diff/1/src/interpreter/interpreter...
> src/interpreter/interpreter.cc:238: : public
> AstTraversalVisitor<CollectEagerFunctionLiterals> {
> On 2016/10/07 at 14:26:34, rmcilroy wrote:
> > I'd really prefer we do this somewhere else - we already walk the AST too
many
> times IMO. Can't we do this in ast-renumbering or something?
> 
> none of the other visitors visit the entire AST (other than the bytecode
> generator of course) ...

How do you do the renumbering / rewriting for the inner functions before
compiling them with the bytecode generator then?

[jochen (gone - plz use gerrit), 2016-10-07 15:40:49]

... new patch set. Still doesn't work.

https://codereview.chromium.org/2399463008/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2399463008/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:307: Compiler::Renumber(parse_info.get());
^^^ like this

[jochen (gone - plz use gerrit), 2016-10-11 16:50:09]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 16:50:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2016-10-11 16:50:41]

now it works!

[jochen (gone - plz use gerrit), 2016-10-11 16:51:12]

jochen@chromium.org changed reviewers:
+ bmeurer@chromium.org

[jochen (gone - plz use gerrit), 2016-10-11 16:51:12]

+bmeurer while everybody else is out :-/

[commit-bot: I haz the power, 2016-10-11 17:27:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 17:27:56]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-10-12 03:25:23]

You really need appropriate reviewers for this. I'm not familiar with any of
this.

[Michael Starzinger, 2016-10-17 13:16:04]

I have to admit that I haven't fully grasped the lifetime management of
CompilationInfo and ParseInfo in this CL. I need to dig into this some more.
Here is a first round of comment until then.

https://codereview.chromium.org/2399463008/diff/60001/src/ast/collect-eager-l...
File src/ast/collect-eager-literals.cc (right):

https://codereview.chromium.org/2399463008/diff/60001/src/ast/collect-eager-l...
src/ast/collect-eager-literals.cc:23: if (variable->location() ==
VariableLocation::UNALLOCATED) {
Why have this special handling here? Any FunctionDeclaration will evaluate the
function literal one way or the other. Wouldn't it be sufficient to just have
the default visitation function for function declarations and let everything
just go through VisitFunctionLiteral below?

https://codereview.chromium.org/2399463008/diff/60001/src/ast/collect-eager-l...
src/ast/collect-eager-literals.cc:27: return;
nit: Why return here? Shouldn't we always fall through to
AstTraversalVisitor::VisitFunctionDeclaration just to play it safe? If we ever
add sub-expressions of any kind to a FunctionLiteral it would stop working
otherwise.

https://codereview.chromium.org/2399463008/diff/60001/src/ast/collect-eager-l...
File src/ast/collect-eager-literals.h (right):

https://codereview.chromium.org/2399463008/diff/60001/src/ast/collect-eager-l...
src/ast/collect-eager-literals.h:22: explicit
CollectEagerLiterals(CompilationInfo* info);
nit: How would you feel about passing just the ParseInfo into the constructor
(or alternatively ParseInfo+Isolate)? That way it would be clear that only a
parse-result is needed for this.

https://codereview.chromium.org/2399463008/diff/60001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2399463008/diff/60001/src/compiler.cc#newcode...
src/compiler.cc:1805: return existing;
nit: Please add a DCHECK(!outer_info->is_debug()) before this early return.

[jochen (gone - plz use gerrit), 2016-10-17 13:19:10]

https://codereview.chromium.org/2399463008/diff/60001/src/ast/collect-eager-l...
File src/ast/collect-eager-literals.cc (right):

https://codereview.chromium.org/2399463008/diff/60001/src/ast/collect-eager-l...
src/ast/collect-eager-literals.cc:23: if (variable->location() ==
VariableLocation::UNALLOCATED) {
On 2016/10/17 at 13:16:03, Michael Starzinger wrote:
> Why have this special handling here? Any FunctionDeclaration will evaluate the
function literal one way or the other. Wouldn't it be sufficient to just have
the default visitation function for function declarations and let everything
just go through VisitFunctionLiteral below?

VisitFunctionLiteral doesn't visit the declarations of the function, so it's
possible that it won't see a function that is only declared in that scope, and
only used in a lazily parsed scope

https://codereview.chromium.org/2399463008/diff/60001/src/ast/collect-eager-l...
src/ast/collect-eager-literals.cc:27: return;
On 2016/10/17 at 13:16:03, Michael Starzinger wrote:
> nit: Why return here? Shouldn't we always fall through to
AstTraversalVisitor::VisitFunctionDeclaration just to play it safe? If we ever
add sub-expressions of any kind to a FunctionLiteral it would stop working
otherwise.

if a function doesn't eager compiler, it might not have a body(), however, the
AstTraversalVisitor blindly visits the body...

[Michael Starzinger, 2016-10-17 13:37:49]

https://codereview.chromium.org/2399463008/diff/60001/src/ast/collect-eager-l...
File src/ast/collect-eager-literals.cc (right):

https://codereview.chromium.org/2399463008/diff/60001/src/ast/collect-eager-l...
src/ast/collect-eager-literals.cc:23: if (variable->location() ==
VariableLocation::UNALLOCATED) {
On 2016/10/17 13:19:10, jochen wrote:
> On 2016/10/17 at 13:16:03, Michael Starzinger wrote:
> > Why have this special handling here? Any FunctionDeclaration will evaluate
the
> function literal one way or the other. Wouldn't it be sufficient to just have
> the default visitation function for function declarations and let everything
> just go through VisitFunctionLiteral below?
> 
> VisitFunctionLiteral doesn't visit the declarations of the function, so it's
> possible that it won't see a function that is only declared in that scope, and
> only used in a lazily parsed scope

Not sure I fully understand your response,
AstTraversalVisitor::VisitFunctionLiteral _does_ visit scope->declarations()
AFAICT. Can you elaborate?

https://codereview.chromium.org/2399463008/diff/60001/src/ast/collect-eager-l...
src/ast/collect-eager-literals.cc:27: return;
On 2016/10/17 13:19:10, jochen wrote:
> On 2016/10/17 at 13:16:03, Michael Starzinger wrote:
> > nit: Why return here? Shouldn't we always fall through to
> AstTraversalVisitor::VisitFunctionDeclaration just to play it safe? If we ever
> add sub-expressions of any kind to a FunctionLiteral it would stop working
> otherwise.
> 
> if a function doesn't eager compiler, it might not have a body(), however, the
> AstTraversalVisitor blindly visits the body...

Then this should go into VisitFunctionLiteral below and basically read "if
(fun->body() == nullptr) return" there IMHO. One could even argue that this is a
"bug" in AstTraversalVisitor in the first place and the handling should go
there. WDYT?