[kernel] Insert kernel bodies into VM heap

[kernel] Insert kernel bodies into VM heap

This CL copies the kernel bodies for all functions and
fields into the VM heap. The function bodies in the VM
heap are then used when compiling the flowgraphs.
This theoretically means that the malloc'd data can be
freed and that snapshotting from kernel could possibly
work, though it hasn't been tested.

R=kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/6c2232cb602818f3de1f80a66a75efa0bf15e0cc

[jensj, 2017-07-12 08:24:49]

jensj@google.com changed reviewers:
+ kmillikin@google.com, sivachandra@google.com

[jensj, 2017-07-12 08:24:50]

Status: Only kernel_reader.cc should read from the malloc'd memory. The
flowgraph builder etc reads from the TypedData.

Known issues:

* At least for now, this means that source code and line starts are created
eagerly. Even worse it is currently read ineffectively.

* To gather token positions (for debugging) it goes through all functions,
fields and classes. That might not be pretty, but it seems to work.

* I couldn't easily free the malloc'd memory - there are 3 different kernel
programs in play (I think 'platform', 'service isolate' and then the actual user
script) - one of them is used more than once (I think that was 'platform').

* Script has an intptr_t field (kernel_script_index) which I don't think is
currently written out when snapshotting. It should be, or stuff will break
(though I think it's only debugging stuff).

I think all of those can be addressed in another CL though.

Caveat: Notice that this builds on top of CL 2973633002 which hasn't landed yet.
Kevin will you take a look at that one too?

[jensj, 2017-07-12 09:01:08]

Description was changed from

==========
[kernel] Insert kernel bodies into VM heap

Description to come.
==========

to

==========
[kernel] Insert kernel bodies into VM heap

This CL copies the kernel bodies for all functions and
fields into the VM heap. The function bodies in the VM
heap are then used when compiling the flowgraphs.
This theoretically means that the malloc'd data can be
freed and that snapshotting from kernel could possibly
work, though it hasn't been tested.
==========

[jensj, 2017-08-03 13:05:47]

On 2017/07/12 08:24:50, jensj wrote:
> Status: Only kernel_reader.cc should read from the malloc'd memory. The
> flowgraph builder etc reads from the TypedData.
> 
> Known issues:
> 
> * At least for now, this means that source code and line starts are created
> eagerly. Even worse it is currently read ineffectively.
> 
> * To gather token positions (for debugging) it goes through all functions,
> fields and classes. That might not be pretty, but it seems to work.
> 
> * I couldn't easily free the malloc'd memory - there are 3 different kernel
> programs in play (I think 'platform', 'service isolate' and then the actual
user
> script) - one of them is used more than once (I think that was 'platform').
> 
> * Script has an intptr_t field (kernel_script_index) which I don't think is
> currently written out when snapshotting. It should be, or stuff will break
> (though I think it's only debugging stuff).
> 
> I think all of those can be addressed in another CL though.
> 
> Caveat: Notice that this builds on top of CL 2973633002 which hasn't landed
yet.
> Kevin will you take a look at that one too?

The last known issue (about the field in Script) is being fixed in 2993613002.

[Kevin Millikin (Google), 2017-08-09 12:08:52]

Complicated, but lgtm.

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_binary.h
File runtime/vm/kernel_binary.h (right):

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.h:151: const uint8_t* buffer_ = buffer();
The underscore (_) suffix is used for member variables, not local variables
(except sometimes in rare cases that you don't want to know about).

You should rename buffer_.  I suggest changing buffer_ to buffer (and then you
might need to call buffer() like this->buffer()).

It should also be OK to just call the accessor buffer() everywhere.

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.h:162: const uint8_t* buffer_ = buffer();
Same here.

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

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:55: : parameters_count_(parameters_count),
parameter_count_

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:433: field.set_kernel_body(body_data);
kernel_body (and body_data) are not the correct names.  Fields don't have
bodies.  I suggest just 'kernel_data' though it's kind of generic.

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:484: function.set_kernel_body(body_data);
kernel_body (and body_data) are misleading names.  It is everything from the
start of the constructor to the end, which includes more than just the body.  I
suggest just 'kernel_data'.

[jensj, 2017-08-10 07:37:34]

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_binary.h
File runtime/vm/kernel_binary.h (right):

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.h:151: const uint8_t* buffer_ = buffer();
On 2017/08/09 12:08:51, Kevin Millikin (Google) wrote:
> The underscore (_) suffix is used for member variables, not local variables
> (except sometimes in rare cases that you don't want to know about).
> 
> You should rename buffer_.  I suggest changing buffer_ to buffer (and then you
> might need to call buffer() like this->buffer()).
> 
> It should also be OK to just call the accessor buffer() everywhere.

Done.

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.h:162: const uint8_t* buffer_ = buffer();
On 2017/08/09 12:08:51, Kevin Millikin (Google) wrote:
> Same here.

Done.

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

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:55: : parameters_count_(parameters_count),
On 2017/08/09 12:08:51, Kevin Millikin (Google) wrote:
> parameter_count_

Done.

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:433: field.set_kernel_body(body_data);
On 2017/08/09 12:08:52, Kevin Millikin (Google) wrote:
> kernel_body (and body_data) are not the correct names.  Fields don't have
> bodies.  I suggest just 'kernel_data' though it's kind of generic.

Done.

https://codereview.chromium.org/2972343002/diff/60001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:484: function.set_kernel_body(body_data);
On 2017/08/09 12:08:52, Kevin Millikin (Google) wrote:
> kernel_body (and body_data) are misleading names.  It is everything from the
> start of the constructor to the end, which includes more than just the body. 
I
> suggest just 'kernel_data'.

Done.

[jensj, 2017-08-10 07:38:23]

Description was changed from

==========
[kernel] Insert kernel bodies into VM heap

This CL copies the kernel bodies for all functions and
fields into the VM heap. The function bodies in the VM
heap are then used when compiling the flowgraphs.
This theoretically means that the malloc'd data can be
freed and that snapshotting from kernel could possibly
work, though it hasn't been tested.
==========

to

==========
[kernel] Insert kernel bodies into VM heap

This CL copies the kernel bodies for all functions and
fields into the VM heap. The function bodies in the VM
heap are then used when compiling the flowgraphs.
This theoretically means that the malloc'd data can be
freed and that snapshotting from kernel could possibly
work, though it hasn't been tested.

R=kmillikin@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6c2232cb602818f3de1f80a66a75efa0bf15e0cc
==========

[jensj, 2017-08-10 07:38:24]

Committed patchset #5 (id:80001) manually as
6c2232cb602818f3de1f80a66a75efa0bf15e0cc (presubmit successful).