Assign external sizes to external strings and to external typed data created by the VM.

Assign external sizes to external strings and to external typed data created by the VM.

Observatory used to report that dart2js hello was using no external memory, but now reports 1.3MB of external memory (all token streams).

R=asiva@google.com

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

[rmacnak, 2016-08-26 23:25:18]

Description was changed from

==========
Add some missing external sizes.
==========

to

==========
Assign external sizes to external strings and to external typed data created by
the VM.
==========

[rmacnak, 2016-08-26 23:30:40]

Description was changed from

==========
Assign external sizes to external strings and to external typed data created by
the VM.
==========

to

==========
Assign external sizes to external strings and to external typed data created by
the VM.

Observatory used to report that dart2js hello was using no external memory, but
now reports 1.3MB of external memory (all token streams).
==========

[rmacnak, 2016-08-26 23:32:14]

Description was changed from

==========
Assign external sizes to external strings and to external typed data created by
the VM.

Observatory used to report that dart2js hello was using no external memory, but
now reports 1.3MB of external memory (all token streams).
==========

to

==========
Assign external sizes to external strings and to external typed data created by
the VM.

Observatory used to report that dart2js hello was using no external memory, but
now reports 1.3MB of external memory (all token streams).
==========

[rmacnak, 2016-08-26 23:32:56]

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

[siva, 2016-08-29 23:03:46]

LGTM with some comments.

https://codereview.chromium.org/2275803002/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2275803002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8510: stream.AddFinalizer(data.GetStream(), DataFinalizer,
external_size);
In this case we are not allocating a new buffer but instead just pointing to the
stream in the snapshot, shouldn't external_size be 0 in this case.

https://codereview.chromium.org/2275803002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:20504: external_size = str_length;
Shouldn't external_size just be 'length' as that is the size passed in from the
external call and it can potentially be larger than str_length.

https://codereview.chromium.org/2275803002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:21002: intptr_t external_size = str.Length();
For the same reason I am wondering if OneByteString::SetPeer and
TwoByteString::SetPeer should have a length argument that we pass in from the
API call implementation.

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

https://codereview.chromium.org/2275803002/diff/40001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2639: return obj.raw();
This whole function should be a NOP or UNREACHABLE right because we never
serialize an ExternalTypedData object as an ExternalTypedData object right?

[rmacnak, 2016-08-30 20:49:17]

PTAL

https://codereview.chromium.org/2275803002/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2275803002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8510: stream.AddFinalizer(data.GetStream(), DataFinalizer,
external_size);
On 2016/08/29 23:03:45, siva wrote:
> In this case we are not allocating a new buffer but instead just pointing to
the
> stream in the snapshot, shouldn't external_size be 0 in this case.

DataFinalizer here does a free, so this can't be pointing into the snapshot.

This stream was allocated by CompressedTokenStreamData::realloc.

The one that points into the snapshot is from clustered_snapshot.cc

https://codereview.chromium.org/2275803002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:20504: external_size = str_length;
On 2016/08/29 23:03:45, siva wrote:
> Shouldn't external_size just be 'length' as that is the size passed in from
the
> external call and it can potentially be larger than str_length.

Yes. Renamed external_size here and external_allocation_size in the
Dart_MakeStringExternal for consistency with Dart_NewWeakPersistentHandle

https://codereview.chromium.org/2275803002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:21002: intptr_t external_size = str.Length();
On 2016/08/29 23:03:45, siva wrote:
> For the same reason I am wondering if OneByteString::SetPeer and
> TwoByteString::SetPeer should have a length argument that we pass in from the
> API call implementation.

Pushed through from Dart_MakeExternalString.

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

https://codereview.chromium.org/2275803002/diff/40001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2639: return obj.raw();
On 2016/08/29 23:03:45, siva wrote:
> This whole function should be a NOP or UNREACHABLE right because we never
> serialize an ExternalTypedData object as an ExternalTypedData object right?

This is never generated from raw_snapshot_object.cc, but it is generated from
PostCObject.

[siva, 2016-09-01 00:12:58]

lgtm

[rmacnak, 2016-09-01 17:56:45]

Description was changed from

==========
Assign external sizes to external strings and to external typed data created by
the VM.

Observatory used to report that dart2js hello was using no external memory, but
now reports 1.3MB of external memory (all token streams).
==========

to

==========
Assign external sizes to external strings and to external typed data created by
the VM.

Observatory used to report that dart2js hello was using no external memory, but
now reports 1.3MB of external memory (all token streams).

R=asiva@google.com

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

[rmacnak, 2016-09-01 17:56:46]

Committed patchset #5 (id:80001) manually as
3aa65a66d26069da31435c188c7fd15a2e5aaa8a (presubmit successful).