Refactor snapshots pieces to include a section for loading instructions into the heap of a regular …

Refactor snapshots pieces to include a section for loading instructions into the heap of a regular isolate.

Progress toward allowing each isolate to load a different snapshot.

R=asiva@google.com

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

[rmacnak, 2017-01-18 01:51:45]

rmacnak@google.com changed reviewers:
+ asiva@google.com

[siva, 2017-01-20 22:53:54]

LGTM with some comments. The page type change could be done in another CL to
avoid making this even bigger.

https://codereview.chromium.org/2622053002/diff/160001/runtime/bin/gen_snapsh...
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/2622053002/diff/160001/runtime/bin/gen_snapsh...
runtime/bin/gen_snapshot.cc:670: "\n");
Contents of the Usage message seems to be outdated and needs an update.

https://codereview.chromium.org/2622053002/diff/160001/runtime/include/dart_a...
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/2622053002/diff/160001/runtime/include/dart_a...
runtime/include/dart_api.h:3262: *  Creates a snapshot that caches unoptimized
code and type feedback for faster
it is both unoptimized and optimized code right

https://codereview.chromium.org/2622053002/diff/160001/runtime/include/dart_a...
runtime/include/dart_api.h:3266: *  instructions_blob and rodata_blob should be
passed as arguments to
The comment here needs an update

https://codereview.chromium.org/2622053002/diff/160001/runtime/vm/gc_sweeper.cc
File runtime/vm/gc_sweeper.cc (right):

https://codereview.chromium.org/2622053002/diff/160001/runtime/vm/gc_sweeper....
runtime/vm/gc_sweeper.cc:22: }
I am wondering if it would make it more clear if we choose
to use 4 types of pages
kData, kExecutable, kReadOnlyData and kReadOnlyExecutable

This could then be
if (page->IsReadOnly()) {
  return true;
}

where Readonly is one of kReadOnlyData or kReadOnlyExecutable

https://codereview.chromium.org/2622053002/diff/160001/runtime/vm/pages.h
File runtime/vm/pages.h (right):

https://codereview.chromium.org/2622053002/diff/160001/runtime/vm/pages.h#new...
runtime/vm/pages.h:31: enum PageType { kData = 0, kExecutable, kNumPageTypes };
See note in other file about page types.

https://codereview.chromium.org/2622053002/diff/160001/runtime/vm/pages.h#new...
runtime/vm/pages.h:235: void VisitObjectsExternalPages(ObjectVisitor* visitor)
const;
The external page name here is confusing with the external size
we use for tracking the space allocated by embedder which is associated with
weak handles.

https://codereview.chromium.org/2622053002/diff/160001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/2622053002/diff/160001/runtime/vm/snapshot.cc...
runtime/vm/snapshot.cc:780: WriteROData(clustered_stream);
These two calls (HandlifyObjects and WriteROData) are back to back always why
not combine them into one function?

Since HandlifyObjects creates regular scoped handles there is a danger that
somebody might modify this code at some point as
{
  HANDLESCOPE(thread);
  HandlifyObjects();
  ...
  ....
}
WriteROData(...);

https://codereview.chromium.org/2622053002/diff/160001/runtime/vm/snapshot.cc...
runtime/vm/snapshot.cc:905: WriteROData(clustered_stream);
Ditto comment.

[rmacnak, 2017-01-21 00:50:43]

https://codereview.chromium.org/2622053002/diff/160001/runtime/bin/gen_snapsh...
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/2622053002/diff/160001/runtime/bin/gen_snapsh...
runtime/bin/gen_snapshot.cc:670: "\n");
On 2017/01/20 22:53:53, siva wrote:
> Contents of the Usage message seems to be outdated and needs an update.

Done.

https://codereview.chromium.org/2622053002/diff/160001/runtime/include/dart_a...
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/2622053002/diff/160001/runtime/include/dart_a...
runtime/include/dart_api.h:3262: *  Creates a snapshot that caches unoptimized
code and type feedback for faster
On 2017/01/20 22:53:54, siva wrote:
> it is both unoptimized and optimized code right

Done.

https://codereview.chromium.org/2622053002/diff/160001/runtime/include/dart_a...
runtime/include/dart_api.h:3266: *  instructions_blob and rodata_blob should be
passed as arguments to
On 2017/01/20 22:53:54, siva wrote:
> The comment here needs an update

Done.

Also updated comment for the AOT case.

https://codereview.chromium.org/2622053002/diff/160001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/2622053002/diff/160001/runtime/vm/snapshot.cc...
runtime/vm/snapshot.cc:780: WriteROData(clustered_stream);
On 2017/01/20 22:53:54, siva wrote:
> These two calls (HandlifyObjects and WriteROData) are back to back always why
> not combine them into one function?
> 
> Since HandlifyObjects creates regular scoped handles there is a danger that
> somebody might modify this code at some point as
> {
>   HANDLESCOPE(thread);
>   HandlifyObjects();
>   ...
>   ....
> }
> WriteROData(...);

Commoned this sequence in one InstructionsWriter::Write and moved the remaining
the following parts to Assemebly|BlobsInstructionsWriter::WriteText.

[rmacnak, 2017-01-23 18:25:10]

Description was changed from

==========
Refactor snapshots pieces to include a section for loading instructions into the
heap of a regular isolate.

Progress toward allowing each isolate to load a different snapshot.
==========

to

==========
Refactor snapshots pieces to include a section for loading instructions into the
heap of a regular isolate.

Progress toward allowing each isolate to load a different snapshot.

R=asiva@google.com

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

[rmacnak, 2017-01-23 18:25:11]

Committed patchset #12 (id:220001) manually as
b46af1e75f9d124ce0e5f8226d570e93ac87d3a9 (presubmit successful).