Toward precompiled snapshots.

Toward precompiled snapshots.

R=asiva@google.com, fschneider@google.com

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

[rmacnak, 2015-08-13 00:56:22]

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

[rmacnak, 2015-08-13 00:56:22]

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

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/snapshot_ids...
runtime/vm/snapshot_ids.h:16: kTransitionSentinelObject,
Referenced from field initializer functions.

[Florian Schneider, 2015-08-18 11:54:14]

lgtm

lgtm.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:698: intptr_t num_flds = to -
func.raw()->from();
This loop setting all the fields seems to be duplicated a lot in this file.
Maybe factor this out into a helper?

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

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:2385: int32_t GetInstructionsId(RawInstructions*
instructions) {
Where is this function used?

There is already SnapshotWriter::GetInstructionsId.

[siva, 2015-08-19 19:40:03]

lgtm with a few comments.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:696: RawObject** to = reader->snapshot_code()
? func.raw()->to()
maybe use a different name instead of 'to' say 'toaddr' or something like that.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1245: }
Why not do this check on top before writing anything.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1283: }
Why is this scoped like this.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1323: }
case statements seem to be not indented.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1325: UNREACHABLE();
It is probably better to throw exceptions in these cases instead of an
UNREACHABLE().

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1365: }
indentation seems to be off.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1379: PcDescriptors::New(length));
Maybe in another CL you could convert these to also do uninitialized allocation.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1746: reader->ArrayHandle()->raw());
This is being read as a reference but the write side seems to write it out
inlined and will lead to a mismatch,

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

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1875: if (!snapshot_code() && cid == kCodeCid) {
maybe
if ((cid == kCodeCid) && !snapshot_code()) {
}

as snapshot_code() will be false mostly

[rmacnak, 2015-08-19 22:33:54]

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:696: RawObject** to = reader->snapshot_code()
? func.raw()->to()
On 2015/08/19 19:40:03, siva wrote:
> maybe use a different name instead of 'to' say 'toaddr' or something like
that.

Done.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:698: intptr_t num_flds = to -
func.raw()->from();
On 2015/08/18 11:54:14, Florian Schneider wrote:
> This loop setting all the fields seems to be duplicated a lot in this file.
> Maybe factor this out into a helper?

Good point, we have a helper for the write. Will put in a later CL.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1245: }
On 2015/08/19 19:40:02, siva wrote:
> Why not do this check on top before writing anything.

Moved up.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1283: }
On 2015/08/19 19:40:02, siva wrote:
> Why is this scoped like this.

Scoping the comment.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1323: }
On 2015/08/19 19:40:03, siva wrote:
> case statements seem to be not indented.

Done.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1325: UNREACHABLE();
On 2015/08/19 19:40:02, siva wrote:
> It is probably better to throw exceptions in these cases instead of an
> UNREACHABLE().

Acknowledged.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:1746: reader->ArrayHandle()->raw());
On 2015/08/19 19:40:02, siva wrote:
> This is being read as a reference but the write side seems to write it out
> inlined and will lead to a mismatch,

Switched write to reference.

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

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1875: if (!snapshot_code() && cid == kCodeCid) {
On 2015/08/19 19:40:03, siva wrote:
> maybe
> if ((cid == kCodeCid) && !snapshot_code()) {
> }
> 
> as snapshot_code() will be false mostly 

Done.

https://codereview.chromium.org/1290933002/diff/20001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:2385: int32_t GetInstructionsId(RawInstructions*
instructions) {
On 2015/08/18 11:54:14, Florian Schneider wrote:
> Where is this function used?
> 
> There is already SnapshotWriter::GetInstructionsId.

Refactoring leftover - deleted.

[rmacnak, 2015-08-19 22:34:22]

Committed patchset #3 (id:40001) manually as
7055e4862bf8ff78ddf060efbccba89aface8fad (presubmit successful).