Make core snapshot word-size portable again.

Make core snapshots word-size portable again.

Flutter creates a single core snapshot (from x64) that is used by both sky_snapshot (x64) and the engine (arm).

R=asiva@google.com

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

[rmacnak, 2016-07-07 23:53:51]

Description was changed from

==========
different approach


x


one

BUG=
==========

to

==========
Make core snapshot word-size portable again.
==========

[rmacnak, 2016-07-07 23:55:34]

Description was changed from

==========
Make core snapshot word-size portable again.
==========

to

==========
Make core snapshot word-size portable again.

Flutter creates a single core snapshot (from x64) that is used by both
sky_snapshot (x64) and the engine (arm).
==========

[rmacnak, 2016-07-08 00:01:29]

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

[rmacnak, 2016-07-08 00:15:13]

Description was changed from

==========
Make core snapshot word-size portable again.

Flutter creates a single core snapshot (from x64) that is used by both
sky_snapshot (x64) and the engine (arm).
==========

to

==========
Make core snapshots word-size portable again.

Flutter creates a single core snapshot (from x64) that is used by both
sky_snapshot (x64) and the engine (arm).
==========

[rmacnak, 2016-07-08 00:57:13]

+2% size, roughly equal / slightly faster

[siva, 2016-07-08 16:25:20]

https://codereview.chromium.org/2132833002/diff/100001/runtime/platform/asser...
File runtime/platform/assert.cc (right):

https://codereview.chromium.org/2132833002/diff/100001/runtime/platform/asser...
runtime/platform/assert.cc:39: fflush(stderr);
OS::PrintErr does a fflush of the stream, this fflush can be removed.

https://codereview.chromium.org/2132833002/diff/100001/runtime/vm/clustered_s...
File runtime/vm/clustered_snapshot.cc (right):

https://codereview.chromium.org/2132833002/diff/100001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:350: s->Write<int32_t>(count);
How do you ensure that the count of objects being written fits in an int32_t
value? Is there are check somewhere when you are tracing through the objects?

https://codereview.chromium.org/2132833002/diff/100001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:3182: }
I am not sure I understand the benefit of classifying these into these 3
categories instead of just writing them out always as int64_t values and
always treating Mints that get created on the other side as always being
canonical and create the objects using Mint::NewCanonical(value).

https://codereview.chromium.org/2132833002/diff/100001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:3185: void WriteFill(Serializer* s) { }
How is the canonical property of the always_mint objects carried over?

https://codereview.chromium.org/2132833002/diff/100001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:3221: #endif
Would prefer if you did 
int64_t value = d->Read<int64_t>();
if (Smi::IsValid(value) {
  ...
} else {
  ...
  ...
}

instead of the #if defined ....

[rmacnak, 2016-07-08 18:08:58]

https://codereview.chromium.org/2132833002/diff/100001/runtime/platform/asser...
File runtime/platform/assert.cc (right):

https://codereview.chromium.org/2132833002/diff/100001/runtime/platform/asser...
runtime/platform/assert.cc:39: fflush(stderr);
On 2016/07/08 16:25:20, siva wrote:
> OS::PrintErr does a fflush of the stream, this fflush can be removed.

Done.

https://codereview.chromium.org/2132833002/diff/100001/runtime/vm/clustered_s...
File runtime/vm/clustered_snapshot.cc (right):

https://codereview.chromium.org/2132833002/diff/100001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:350: s->Write<int32_t>(count);
On 2016/07/08 16:25:20, siva wrote:
> How do you ensure that the count of objects being written fits in an int32_t
> value? Is there are check somewhere when you are tracing through the objects?

Added check in Serializer::Serialize()

https://codereview.chromium.org/2132833002/diff/100001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:3182: }
On 2016/07/08 16:25:20, siva wrote:
> I am not sure I understand the benefit of classifying these into these 3
> categories instead of just writing them out always as int64_t values and
> always treating Mints that get created on the other side as always being
> canonical and create the objects using Mint::NewCanonical(value).

It is not safe to run Mint::NewCanonical during ReadAlloc. In particular, the
mint canonical table hasn't been allocated yet.

https://codereview.chromium.org/2132833002/diff/100001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:3185: void WriteFill(Serializer* s) { }
On 2016/07/08 16:25:20, siva wrote:
> How is the canonical property of the always_mint objects carried over?

Every encountered mint is canonical.

[rmacnak, 2016-07-08 20:17:54]

https://codereview.chromium.org/2132833002/diff/100001/runtime/vm/clustered_s...
File runtime/vm/clustered_snapshot.cc (right):

https://codereview.chromium.org/2132833002/diff/100001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:3221: #endif
On 2016/07/08 16:25:20, siva wrote:
> Would prefer if you did 
> int64_t value = d->Read<int64_t>();
> if (Smi::IsValid(value) {
>   ...
> } else {
>   ...
>   ...
> }
> 
> instead of the #if defined ....

Done.

[siva, 2016-07-08 21:10:00]

lgtm

https://codereview.chromium.org/2132833002/diff/140001/runtime/vm/clustered_s...
File runtime/vm/clustered_snapshot.cc (right):

https://codereview.chromium.org/2132833002/diff/140001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:3195: Array::Handle(zone,
Array::MakeArray(new_constants));
Not for this CL maybe the next one.

You could count the number of canonical Mint objects that were deserialized in
the ReadAlloc method above and create an Array of that size instead of the first
creating a growable array and then making it an array. You should also allocate
that object straight in the old space.

https://codereview.chromium.org/2132833002/diff/140001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:4290: #endif
How does this protect against overflows on a 32 bit machine, i.e
num_base_objects + num_written_objects overflows.

[rmacnak, 2016-07-08 21:18:22]

https://codereview.chromium.org/2132833002/diff/140001/runtime/vm/clustered_s...
File runtime/vm/clustered_snapshot.cc (right):

https://codereview.chromium.org/2132833002/diff/140001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:3195: Array::Handle(zone,
Array::MakeArray(new_constants));
On 2016/07/08 21:10:00, siva wrote:
> Not for this CL maybe the next one.
> 
> You could count the number of canonical Mint objects that were deserialized in
> the ReadAlloc method above and create an Array of that size instead of the
first
> creating a growable array and then making it an array. You should also
allocate
> that object straight in the old space.

Noted.

https://codereview.chromium.org/2132833002/diff/140001/runtime/vm/clustered_s...
runtime/vm/clustered_snapshot.cc:4290: #endif
On 2016/07/08 21:10:00, siva wrote:
> How does this protect against overflows on a 32 bit machine, i.e
> num_base_objects + num_written_objects overflows.

It can't overflow on a 32-bit machine because we can't allocate 2^32 objects. An
object can be counted only as either a base or a written object but not both.

[rmacnak, 2016-07-08 21:18:50]

Description was changed from

==========
Make core snapshots word-size portable again.

Flutter creates a single core snapshot (from x64) that is used by both
sky_snapshot (x64) and the engine (arm).
==========

to

==========
Make core snapshots word-size portable again.

Flutter creates a single core snapshot (from x64) that is used by both
sky_snapshot (x64) and the engine (arm).

R=asiva@google.com

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

[rmacnak, 2016-07-08 21:18:52]

Committed patchset #8 (id:140001) manually as
d75699e86338584266c5c051a083a8b4795ac054 (presubmit successful).