Move tar archive parsing to C++

Move tar archive parsing to C++

This gives a 30+ms speedup to startup.

Also uses HashMap instead of Map for assets.

BUG=
R=iposva@google.com

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

[zra, 2015-11-24 20:26:48]

Description was changed from

==========
Move tar archive parsing to C++

This gives a 30+ms speedup to startup.

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

to

==========
Move tar archive parsing to C++

This gives a 30+ms speedup to startup.

Also uses HashMap instead of Map for assets.

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

[zra, 2015-11-24 20:27:14]

zra@google.com changed reviewers:
+ iposva@google.com, turnidge@google.com

[zra, 2015-11-24 23:04:36]

Removed <string> and <vector> as discussed offline.

[Florian Schneider, 2015-11-25 10:20:35]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2015-11-25 10:20:35]

How fast is startup with a gzipped archive?

[Ivan Posva, 2015-11-25 17:09:04]

Next round of comments.

-Ivan

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.cc
File runtime/lib/vmservice.cc (right):

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.c...
runtime/lib/vmservice.cc:155: class ByteStream {
How does this differ from class ReadStream in datastream.h?

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.c...
runtime/lib/vmservice.cc:222: static const intptr_t tarHeaderSize = 512;
enum instead of a set of constants? Then you can also type the return value of
the ReadType() method.

Do the constants have to be public?

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice_p...
File runtime/lib/vmservice_patch.dart (right):

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice_p...
runtime/lib/vmservice_patch.dart:7: /* patch */ static HashMap<String, Asset>
request() {
Does this need to be a public addition to the Asset class?

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice_p...
runtime/lib/vmservice_patch.dart:15: var a = new Asset(assetList[i][0],
assetList[i][1]);
If you have even entries for the names and odd entries for the data, then you do
not need to allocate a list of lists of lists:

var a = new Asset(assetList[i], assetList[i + 1]);

[Ivan Posva, 2015-11-25 17:36:44]

-Ivan

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.cc
File runtime/lib/vmservice.cc (right):

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.c...
runtime/lib/vmservice.cc:364: uint8_t* data = reinterpret_cast<uint8_t*>(peer);
You also should delete the handle here. No?

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.c...
runtime/lib/vmservice.cc:378: data_handle, &typ, &bytes, &length);
As discussed we should check if there is a possibility to just keep the raw
pointer that was read from file (or is in the .text section) and make all
external arrays/strings point into that instead reducing the memory overhead
even further. That would mean that we have to guarantee that the assets data is
not removed from memory.

[zra, 2015-11-25 19:06:16]

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.cc
File runtime/lib/vmservice.cc (right):

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.c...
runtime/lib/vmservice.cc:155: class ByteStream {
On 2015/11/25 17:09:04, Ivan Posva wrote:
> How does this differ from class ReadStream in datastream.h?

There isn't a big difference, and I changed to use ReadStream.

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.c...
runtime/lib/vmservice.cc:222: static const intptr_t tarHeaderSize = 512;
On 2015/11/25 17:09:04, Ivan Posva wrote:
> enum instead of a set of constants? Then you can also type the return value of
> the ReadType() method.
> 
> Do the constants have to be public?

Changed to private enums.

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.c...
runtime/lib/vmservice.cc:364: uint8_t* data = reinterpret_cast<uint8_t*>(peer);
On 2015/11/25 17:36:44, Ivan Posva wrote:
> You also should delete the handle here. No?

Looking at //runtime/bin/io_buffer.h the Finalizer there does not delete the
WeakPersistentHandle.

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.c...
runtime/lib/vmservice.cc:378: data_handle, &typ, &bytes, &length);
On 2015/11/25 17:36:44, Ivan Posva wrote:
> As discussed we should check if there is a possibility to just keep the raw
> pointer that was read from file (or is in the .text section) and make all
> external arrays/strings point into that instead reducing the memory overhead
> even further. That would mean that we have to guarantee that the assets data
is
> not removed from memory.

Filed issue #25041 and added a TODO.

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice_p...
File runtime/lib/vmservice_patch.dart (right):

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice_p...
runtime/lib/vmservice_patch.dart:7: /* patch */ static HashMap<String, Asset>
request() {
On 2015/11/25 17:09:04, Ivan Posva wrote:
> Does this need to be a public addition to the Asset class?

This is patching over a public call implemented in Dart in
//sdk/lib/vmservice/asset.dart.

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice_p...
runtime/lib/vmservice_patch.dart:15: var a = new Asset(assetList[i][0],
assetList[i][1]);
On 2015/11/25 17:09:04, Ivan Posva wrote:
> If you have even entries for the names and odd entries for the data, then you
do
> not need to allocate a list of lists of lists:
> 
> var a = new Asset(assetList[i], assetList[i + 1]);

Done.

[zra, 2015-11-25 20:52:58]

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.cc
File runtime/lib/vmservice.cc (right):

https://codereview.chromium.org/1474603003/diff/20001/runtime/lib/vmservice.c...
runtime/lib/vmservice.cc:364: uint8_t* data = reinterpret_cast<uint8_t*>(peer);
On 2015/11/25 19:06:15, zra wrote:
> On 2015/11/25 17:36:44, Ivan Posva wrote:
> > You also should delete the handle here. No?
> 
> Looking at //runtime/bin/io_buffer.h the Finalizer there does not delete the
> WeakPersistentHandle.

We verified offline that WeakPersistentHandles are deleted by the VM after
calling the finalizer.

[Ivan Posva, 2015-11-25 21:45:02]

LGTM -ip

[zra, 2015-11-25 22:39:21]

Description was changed from

==========
Move tar archive parsing to C++

This gives a 30+ms speedup to startup.

Also uses HashMap instead of Map for assets.

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

to

==========
Move tar archive parsing to C++

This gives a 30+ms speedup to startup.

Also uses HashMap instead of Map for assets.

BUG=
R=iposva@google.com

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

[zra, 2015-11-25 22:39:23]

Committed patchset #3 (id:40001) manually as
5ee95b085fb8233e0df70baa7b5dced6718d9d92 (presubmit successful).