Initial steps into streaming the kernel flowgraph

Initial steps into streaming the kernel flowgraph

- Moved stuff into .h
- Leak kernel binary blob
- "Streaming" rethrow in FlowgraphBuilder

R=kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/63f1de206835a356dcf22e029504c3d9d7107d4c

[jensj, 2017-03-28 11:36:24]

Description was changed from

==========
Initial steps into streaming the kernel flowgraph

- Moved stuff into .h
- "Streaming" rethrow in FlowgraphBuilder
==========

to

==========
Initial steps into streaming the kernel flowgraph

- Moved stuff into .h
- Leak kernel binary blob
- "Streaming" rethrow in FlowgraphBuilder
==========

[Kevin Millikin (Google), 2017-03-28 12:49:59]

kmillikin@google.com changed reviewers:
+ kmillikin@google.com

[Kevin Millikin (Google), 2017-03-28 12:50:00]

Comments.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:110: Fragment instructions;
return Fragment();

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:117: TokenPosition position =
reader_->ReadPosition();
I would take all these functions (KernelReader::ReadPosition,
FlowGraphBuilder::DebugStepCheck, FlowGraphBuilder::catch_block, etc.) and make
implementations of them in KernelFlowGraphBuilder that just delegate to the
underlying implementation.

Eventually, those other classes will be able to be rolled into this one, so we
might as well start coding that way now.  Plus, it reads a lot better.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:118: instructions =
flowGraph_builder_->DebugStepCheck(position);
Fragment instructions = DebugStepCheck(position);

(No reason to initialize an empty fragment only to overwrite it with this one.)

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.h (right):

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:20: class KernelFlowgraphBuilder {
I'd call this something like StreamingFlowGraphBuilder or
BinaryFlowGraphBuilder, because the other one is also a Kernel flow graph
builder so the generic name makes it confusing to talk about and it's easy to
change it later.

Plus, this is kernel::KerneFlowGraphBuilder which seems redundant.

Also be consistent.  We have (apparently) decided that "flow graph" is two words
so this should be capitalized like KernelFlowGraphBuilder.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:33: Fragment buildRethrow();
BuildRethrow()

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:35: FlowGraphBuilder* flowGraph_builder_;
flow_graph_builder_ or possibly flowgraph_builder_ if you change flowgraph to be
one word (I don't recommend it).   Definitely not flowGraph_builder_.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (left):

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:5332: instructions = DebugStepCheck(node->position())
+ instructions;
This is pretty weird.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:1053: Library* library_;
Instead of the Library (which we eventually won't have access to) can you just
put the KernelFlowGraphBuilder here?  If not, could you put the binary (and its
length?)?

[jensj, 2017-03-29 09:30:42]

Thanks, PTAL. Mind the rebase.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:110: Fragment instructions;
On 2017/03/28 12:50:00, Kevin Millikin (Google) wrote:
> return Fragment();

Done.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:117: TokenPosition position =
reader_->ReadPosition();
On 2017/03/28 12:50:00, Kevin Millikin (Google) wrote:
> I would take all these functions (KernelReader::ReadPosition,
> FlowGraphBuilder::DebugStepCheck, FlowGraphBuilder::catch_block, etc.) and
make
> implementations of them in KernelFlowGraphBuilder that just delegate to the
> underlying implementation.
> 
> Eventually, those other classes will be able to be rolled into this one, so we
> might as well start coding that way now.  Plus, it reads a lot better.

Done.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:118: instructions =
flowGraph_builder_->DebugStepCheck(position);
On 2017/03/28 12:50:00, Kevin Millikin (Google) wrote:
> Fragment instructions = DebugStepCheck(position);
> 
> (No reason to initialize an empty fragment only to overwrite it with this
one.)

Done.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.h (right):

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:20: class KernelFlowgraphBuilder {
On 2017/03/28 12:50:00, Kevin Millikin (Google) wrote:
> I'd call this something like StreamingFlowGraphBuilder or
> BinaryFlowGraphBuilder, because the other one is also a Kernel flow graph
> builder so the generic name makes it confusing to talk about and it's easy to
> change it later.
> 
> Plus, this is kernel::KerneFlowGraphBuilder which seems redundant.
> 
> Also be consistent.  We have (apparently) decided that "flow graph" is two
words
> so this should be capitalized like KernelFlowGraphBuilder.

Done.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:33: Fragment buildRethrow();
On 2017/03/28 12:50:00, Kevin Millikin (Google) wrote:
> BuildRethrow()

Done.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:35: FlowGraphBuilder* flowGraph_builder_;
On 2017/03/28 12:50:00, Kevin Millikin (Google) wrote:
> flow_graph_builder_ or possibly flowgraph_builder_ if you change flowgraph to
be
> one word (I don't recommend it).   Definitely not flowGraph_builder_.

Done.

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (left):

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:5332: instructions = DebugStepCheck(node->position())
+ instructions;
On 2017/03/28 12:50:00, Kevin Millikin (Google) wrote:
> This is pretty weird.

Yeah, sorry about that :x

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2776373002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:1053: Library* library_;
On 2017/03/28 12:50:00, Kevin Millikin (Google) wrote:
> Instead of the Library (which we eventually won't have access to) can you just
> put the KernelFlowGraphBuilder here?  If not, could you put the binary (and
its
> length?)?

I for one cannot figure out how to make it compile if I make it
KernelFlowGraphBuilder --- but I can surely make it the binary...

[Kevin Millikin (Google), 2017-03-31 12:07:46]

LGTM.

We will accept leaking the binary for now but I will work on breaking it into
smaller pieces and moving them into the VM's heap so they can be GC'd.

[jensj, 2017-04-03 09:40:47]

Description was changed from

==========
Initial steps into streaming the kernel flowgraph

- Moved stuff into .h
- Leak kernel binary blob
- "Streaming" rethrow in FlowgraphBuilder
==========

to

==========
Initial steps into streaming the kernel flowgraph

- Moved stuff into .h
- Leak kernel binary blob
- "Streaming" rethrow in FlowgraphBuilder

R=kmillikin@google.com

Committed:
https://github.com/dart-lang/sdk/commit/63f1de206835a356dcf22e029504c3d9d7107d4c
==========

[jensj, 2017-04-03 09:40:49]

Committed patchset #5 (id:80001) manually as
63f1de206835a356dcf22e029504c3d9d7107d4c (presubmit successful).