[standalone] Automatically decompress gzip'd resources, including sources and script snapshots.

[standalone] Automatically decompress gzip'd resources, including sources and script snapshots.

Closes #30315

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

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

[rmacnak, 2017-08-03 22:00:33]

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

[rmacnak, 2017-08-03 22:00:34]

I'm not sure about this change. The decompression overhead is quite high.

For the Flutter tool:

uncompressed size  5326676 B
  compressed size  1900940 B
  decompress time       73 ms
 deserialize time      109 ms

[codefu, 2017-08-03 22:05:35]

On 2017/08/03 22:00:34, rmacnak wrote:
> I'm not sure about this change. The decompression overhead is quite high.
> 
> For the Flutter tool:
> 
> uncompressed size  5326676 B
>   compressed size  1900940 B
>   decompress time       73 ms
>  deserialize time      109 ms

Our uncompressed target is 1.1MB. Compresses to 400KB.  I'm sure our decompress
time is higher (lower power proc), but our network conditions are not going to
be great.  Is this not optional? I was imagining snapshot flags would be
"--snapshot --compressed=true", which would generate a .gz file, and load time
wouldn't require a flag if the filename was .gz.

[rmacnak, 2017-08-03 23:47:18]

Description was changed from

==========
[standalone] Compress script snapshots.

Closes #30315
==========

to

==========
[standalone] Automatically decompress gzip'd resources, including sources and
script snapshots.

Closes #30315
==========

[rmacnak, 2017-08-03 23:51:50]

Description was changed from

==========
[standalone] Automatically decompress gzip'd resources, including sources and
script snapshots.

Closes #30315
==========

to

==========
[standalone] Automatically decompress gzip'd resources, including sources and
script snapshots.

Closes #30315
==========

[rmacnak, 2017-08-03 23:54:41]

Adjusted the decompression to happen as needed at the loader level instead of
specifically and unconditionally for snapshots. The makes compression opt-in,
and means we also support loading compressed source files or kernel files.

$ ./out/DebugX64/dart ~/hello.dart
Hello, world!
$ gzip ~/hello.dart
$ ./out/DebugX64/dart ~/hello.dart.gz
Hello, world!
$ gunzip ~/hello.dart.gz 
$ ./out/DebugX64/dart --snapshot=hello.snapshot ~/hello.dart
$ ./out/DebugX64/dart hello.snapshot
Hello, world!
$ gzip hello.snapshot
$ ./out/DebugX64/dart hello.snapshot.gz
Hello, world!

[zra, 2017-08-04 05:17:00]

https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/gzip.cc
File runtime/bin/gzip.cc (right):

https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/gzip.cc#new...
runtime/bin/gzip.cc:61: *output = reinterpret_cast<uint8_t*>(realloc(*output,
*output_length));
This is probably fine when there are only a couple of chunks, but after that it
will be noticeable that this is n^2. 1MB of output is going to hit this 3 times,
and copy 2560KB bytes in addition to the 1MB of copies from chunk_out to
output[output_cursor]. I suspect that this is a good place to look for savings
in the decompression time.

https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/gzip.cc#new...
runtime/bin/gzip.cc:76: void Compress(const uint8_t* input,
Is this used anywhere?

[siva, 2017-08-04 17:11:29]

https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/dfe.cc
File runtime/bin/dfe.cc (right):

https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/dfe.cc#newc...
runtime/bin/dfe.cc:149: if (DartUtils::SniffForMagicNumber(temp,
*kernel_ir_size) !=
Now that SniffMagicNumber does not modify the buffer pointer, the command above
can be removed and the cal could just be
if (DartUtils::SniffForMagicNumber(buffer, *kernel_ir_size)

Another aspect I don't understand is this call updates the kernel_ir_size
variable which is updated back to the caller but the magic number is not skipped
so I am not sure how the kernel reader does not run into an issue because of
that.

[rmacnak, 2017-08-07 18:18:57]

https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/dfe.cc
File runtime/bin/dfe.cc (right):

https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/dfe.cc#newc...
runtime/bin/dfe.cc:149: if (DartUtils::SniffForMagicNumber(temp,
*kernel_ir_size) !=
On 2017/08/04 17:11:29, siva wrote:
> Now that SniffMagicNumber does not modify the buffer pointer, the command
above
> can be removed and the cal could just be
> if (DartUtils::SniffForMagicNumber(buffer, *kernel_ir_size)

Done.

> Another aspect I don't understand is this call updates the kernel_ir_size
> variable which is updated back to the caller but the magic number is not
skipped
> so I am not sure how the kernel reader does not run into an issue because of
> that.

The kernel reader expects to see its magic number, unlike the snapshot reader
that expect the embedder to strip it because the embedder was the one who added
it in the first place.

[rmacnak, 2017-08-09 18:02:37]

On 2017/08/07 18:18:57, rmacnak wrote:
> https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/dfe.cc
> File runtime/bin/dfe.cc (right):
> 
>
https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/dfe.cc#newc...
> runtime/bin/dfe.cc:149: if (DartUtils::SniffForMagicNumber(temp,
> *kernel_ir_size) !=
> On 2017/08/04 17:11:29, siva wrote:
> > Now that SniffMagicNumber does not modify the buffer pointer, the command
> above
> > can be removed and the cal could just be
> > if (DartUtils::SniffForMagicNumber(buffer, *kernel_ir_size)
> 
> Done.
> 
> > Another aspect I don't understand is this call updates the kernel_ir_size
> > variable which is updated back to the caller but the magic number is not
> skipped
> > so I am not sure how the kernel reader does not run into an issue because of
> > that.
> 
> The kernel reader expects to see its magic number, unlike the snapshot reader
> that expect the embedder to strip it because the embedder was the one who
added
> it in the first place.

As discussed offline, the old magic number code would skip over the magic number
for snapshots but not kernel files. The new code never skips over the magic
number.

[siva, 2017-08-09 18:12:33]

LGTM, I guess zra@ had a comment about the growth policy which may need to be
addressed.

[rmacnak, 2017-08-10 21:09:09]

https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/gzip.cc
File runtime/bin/gzip.cc (right):

https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/gzip.cc#new...
runtime/bin/gzip.cc:61: *output = reinterpret_cast<uint8_t*>(realloc(*output,
*output_length));
On 2017/08/04 05:17:00, zra wrote:
> This is probably fine when there are only a couple of chunks, but after that
it
> will be noticeable that this is n^2. 1MB of output is going to hit this 3
times,
> and copy 2560KB bytes in addition to the 1MB of copies from chunk_out to
> output[output_cursor]. I suspect that this is a good place to look for savings
> in the decompression time.

Switched to exponential growth, but it doesn't have a noticeable impact on
decompression time. At least for dart2js's ~3MB script snapshot.

before
realloc 262144
realloc 524288
realloc 786432
realloc 786543
realloc 1048687
realloc 1310831
realloc 1455383
realloc 1717527
realloc 1979671
realloc 2128929
realloc 2391073
realloc 2653217
realloc 2861378
realloc 3123522
realloc 3385666
realloc 3507915

after
realloc 5208116

https://codereview.chromium.org/2991393002/diff/40001/runtime/bin/gzip.cc#new...
runtime/bin/gzip.cc:76: void Compress(const uint8_t* input,
On 2017/08/04 05:17:00, zra wrote:
> Is this used anywhere?

Not anymore, removed.

[zra, 2017-08-10 21:19:20]

lgtm

[rmacnak, 2017-08-11 17:58:07]

Fixed case in decompression where the snapshot is smaller than the chunk size.

[rmacnak, 2017-08-11 19:18:24]

Description was changed from

==========
[standalone] Automatically decompress gzip'd resources, including sources and
script snapshots.

Closes #30315
==========

to

==========
[standalone] Automatically decompress gzip'd resources, including sources and
script snapshots.

Closes #30315

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

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

[rmacnak, 2017-08-11 19:18:25]

Committed patchset #6 (id:100001) manually as
7dfe62f70d0742c09733fdecd36a2e697c8cb190 (presubmit successful).