Compress sources of JS libraries in addition to the snapshot.

Compress sources of JS libraries in addition to the snapshot.
This saves ~170K on current sources.

R=sgjesse@chromium.org
BUG=none
TEST=none

Committed: http://code.google.com/p/v8/source/detail?r=8189

[mnaganov (inactive), 2011-06-06 09:01:43]

+vitalyr

[Søren Thygesen Gjesse, 2011-06-06 09:21:58]

LGTM

http://codereview.chromium.org/7066048/diff/1/src/mksnapshot.cc
File src/mksnapshot.cc (right):

http://codereview.chromium.org/7066048/diff/1/src/mksnapshot.cc#newcode322
src/mksnapshot.cc:322: #ifdef COMPRESS_STARTUP_DATA_BZ2
Can't we use the normal V8 API here?

[Søren Thygesen Gjesse, 2011-06-06 09:21:59]

LGTM

http://codereview.chromium.org/7066048/diff/1/src/mksnapshot.cc
File src/mksnapshot.cc (right):

http://codereview.chromium.org/7066048/diff/1/src/mksnapshot.cc#newcode322
src/mksnapshot.cc:322: #ifdef COMPRESS_STARTUP_DATA_BZ2
Can't we use the normal V8 API here?

[Vitaly Repeshko, 2011-06-06 10:08:23]

http://codereview.chromium.org/7066048/diff/1/samples/shell.cc
File samples/shell.cc (right):

http://codereview.chromium.org/7066048/diff/1/samples/shell.cc#newcode314
samples/shell.cc:314: if (compressed_data[i].compressed_size != 0) {
When does this happen?

http://codereview.chromium.org/7066048/diff/1/samples/shell.cc#newcode326
samples/shell.cc:326: compressed_data[i].data = decompressed;
It seems strange that we assign a block of fresh uninitialized memory here in
case compressed_size is 0.

http://codereview.chromium.org/7066048/diff/1/src/SConscript
File src/SConscript (right):

http://codereview.chromium.org/7066048/diff/1/src/SConscript#newcode337
src/SConscript:337: libraries_src = env.JS2C(['libraries.cc'], library_files,
**BuildJS2CEnv('CORE'))
It's already a lost cause but let's try to fit lines in this file in 80 cols.

http://codereview.chromium.org/7066048/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/7066048/diff/1/src/api.cc#newcode354
src/api.cc:354: for (int i = 0, idx = kSnapshotDataCount;
It may be easier to read to maintain a separate offset variable (updated before
the loops) and replace usages of idx with offset + i.

http://codereview.chromium.org/7066048/diff/1/src/api.cc#newcode355
src/api.cc:355: i < i::Natives::GetBuiltinsCount();
I hope no compiler will have trouble with 'i' both as a variable and a namespace
name.

http://codereview.chromium.org/7066048/diff/1/src/api.cc#newcode391
src/api.cc:391: for (int i = 0, idx = kSnapshotDataCount;
Same here.

http://codereview.chromium.org/7066048/diff/1/src/mksnapshot.cc
File src/mksnapshot.cc (right):

http://codereview.chromium.org/7066048/diff/1/src/mksnapshot.cc#newcode282
src/mksnapshot.cc:282: for (int i = 0; i < i::Natives::GetBuiltinsCount(); ++i)
{
Hmm, it seems annoying to repeat this code with the internal details of Natives
exposed. Can we somehow rework it to take the decompressor as a function?

http://codereview.chromium.org/7066048/diff/1/tools/js2c.py
File tools/js2c.py (right):

http://codereview.chromium.org/7066048/diff/1/tools/js2c.py#newcode354
tools/js2c.py:354: if env['COMPRESSION'] == 'bz2':
I think we can get better compression by gluing together all sources.

http://codereview.chromium.org/7066048/diff/1/tools/js2c.py#newcode355
tools/js2c.py:355: lines = bz2.compress(lines, 9)
9 seems to be the default compression level so we can avoid the magic number.

http://codereview.chromium.org/7066048/diff/1/tools/js2c.py#newcode370
tools/js2c.py:370: raw_source_lines.append(RAW_SOURCE_DECLARATION % { 'raw_id':
raw_id, 'id': id })
Let's at least fit the code lines in 80 cols.

[mnaganov (inactive), 2011-06-06 13:40:18]

Comments addressed, please take another look.

BTW, compressing concatenated sources gives us another ~10K size reduction.
Thanks a lot Vitaly for the suggestion.

http://codereview.chromium.org/7066048/diff/1/samples/shell.cc
File samples/shell.cc (right):

http://codereview.chromium.org/7066048/diff/1/samples/shell.cc#newcode314
samples/shell.cc:314: if (compressed_data[i].compressed_size != 0) {
On 2011/06/06 10:08:23, Vitaly Repeshko wrote:
> When does this happen?

When there is no snapshot. See snapshot-empty.cc.

http://codereview.chromium.org/7066048/diff/1/samples/shell.cc#newcode326
samples/shell.cc:326: compressed_data[i].data = decompressed;
On 2011/06/06 10:08:23, Vitaly Repeshko wrote:
> It seems strange that we assign a block of fresh uninitialized memory here in
> case compressed_size is 0.

We should never read from it in this case. I've added an assertion that in this
case raw_ (decompressed) size, must also be 0.

http://codereview.chromium.org/7066048/diff/1/src/SConscript
File src/SConscript (right):

http://codereview.chromium.org/7066048/diff/1/src/SConscript#newcode337
src/SConscript:337: libraries_src = env.JS2C(['libraries.cc'], library_files,
**BuildJS2CEnv('CORE'))
On 2011/06/06 10:08:23, Vitaly Repeshko wrote:
> It's already a lost cause but let's try to fit lines in this file in 80 cols.

Done.

http://codereview.chromium.org/7066048/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/7066048/diff/1/src/api.cc#newcode354
src/api.cc:354: for (int i = 0, idx = kSnapshotDataCount;
On 2011/06/06 10:08:23, Vitaly Repeshko wrote:
> It may be easier to read to maintain a separate offset variable (updated
before
> the loops) and replace usages of idx with offset + i.

Done.

http://codereview.chromium.org/7066048/diff/1/src/api.cc#newcode355
src/api.cc:355: i < i::Natives::GetBuiltinsCount();
On 2011/06/06 10:08:23, Vitaly Repeshko wrote:
> I hope no compiler will have trouble with 'i' both as a variable and a
namespace
> name.

Bots will tell ;)

http://codereview.chromium.org/7066048/diff/1/src/mksnapshot.cc
File src/mksnapshot.cc (right):

http://codereview.chromium.org/7066048/diff/1/src/mksnapshot.cc#newcode282
src/mksnapshot.cc:282: for (int i = 0; i < i::Natives::GetBuiltinsCount(); ++i)
{
On 2011/06/06 10:08:23, Vitaly Repeshko wrote:
> Hmm, it seems annoying to repeat this code with the internal details of
Natives
> exposed. Can we somehow rework it to take the decompressor as a function?

Converted to API usage (as Soeren suggested), and factored out. Now the same
code is reused here and in sample.cc

http://codereview.chromium.org/7066048/diff/1/src/mksnapshot.cc#newcode322
src/mksnapshot.cc:322: #ifdef COMPRESS_STARTUP_DATA_BZ2
On 2011/06/06 09:22:01, Søren Gjesse wrote:
> Can't we use the normal V8 API here?

Done.

http://codereview.chromium.org/7066048/diff/1/tools/js2c.py
File tools/js2c.py (right):

http://codereview.chromium.org/7066048/diff/1/tools/js2c.py#newcode354
tools/js2c.py:354: if env['COMPRESSION'] == 'bz2':
On 2011/06/06 10:08:23, Vitaly Repeshko wrote:
> I think we can get better compression by gluing together all sources.

Good idea! This also simplifies code.

http://codereview.chromium.org/7066048/diff/1/tools/js2c.py#newcode355
tools/js2c.py:355: lines = bz2.compress(lines, 9)
On 2011/06/06 10:08:23, Vitaly Repeshko wrote:
> 9 seems to be the default compression level so we can avoid the magic number.

Done.

http://codereview.chromium.org/7066048/diff/1/tools/js2c.py#newcode370
tools/js2c.py:370: raw_source_lines.append(RAW_SOURCE_DECLARATION % { 'raw_id':
raw_id, 'id': id })
On 2011/06/06 10:08:23, Vitaly Repeshko wrote:
> Let's at least fit the code lines in 80 cols.

Done.

[mnaganov (inactive), 2011-06-06 13:41:50]

+satish

[Søren Thygesen Gjesse, 2011-06-06 14:30:56]

LGTM

[Vitaly Repeshko, 2011-06-06 15:59:15]

http://codereview.chromium.org/7066048/diff/7001/samples/shell.cc
File samples/shell.cc (right):

http://codereview.chromium.org/7066048/diff/7001/samples/shell.cc#newcode312
samples/shell.cc:312: BZip2Decompressor startup_data_decompressor;
It seems unfortunate that the shell sample is using something that looks like
internal code to demonstrate the compression feature. Could you document that
it's safe to use its implementation details? Or how about we expose a documented
convenience API DecompressWithCallback?

http://codereview.chromium.org/7066048/diff/7001/src/bz2-decompress.h
File src/bz2-decompress.h (right):

http://codereview.chromium.org/7066048/diff/7001/src/bz2-decompress.h#newcode31
src/bz2-decompress.h:31: class BZip2Decompressor {
This should be some namespace and should be documented.

[mnaganov (inactive), 2011-06-06 20:37:37]

A good idea!

I've introduced a base class StartupDataDecompressor in the V8 API that calls
API functions in the right sequence. An embedder only needs to subclass from it
and provide the decompressing function. This is what I'm doing in shell.cc and
mksnapshot.cc. A little code duplication has emerged between them, but I think
this is acceptable.

On 2011/06/06 15:59:15, Vitaly Repeshko wrote:
> http://codereview.chromium.org/7066048/diff/7001/samples/shell.cc
> File samples/shell.cc (right):
> 
> http://codereview.chromium.org/7066048/diff/7001/samples/shell.cc#newcode312
> samples/shell.cc:312: BZip2Decompressor startup_data_decompressor;
> It seems unfortunate that the shell sample is using something that looks like
> internal code to demonstrate the compression feature. Could you document that
> it's safe to use its implementation details? Or how about we expose a
documented
> convenience API DecompressWithCallback?
> 
> http://codereview.chromium.org/7066048/diff/7001/src/bz2-decompress.h
> File src/bz2-decompress.h (right):
> 
>
http://codereview.chromium.org/7066048/diff/7001/src/bz2-decompress.h#newcode31
> src/bz2-decompress.h:31: class BZip2Decompressor {
> This should be some namespace and should be documented.

[Søren Thygesen Gjesse, 2011-06-07 05:53:25]

On 2011/06/06 20:37:37, Mikhail Naganov (Chromium) wrote:
> A good idea!
> 
> I've introduced a base class StartupDataDecompressor in the V8 API that calls
> API functions in the right sequence. An embedder only needs to subclass from
it
> and provide the decompressing function. This is what I'm doing in shell.cc and
> mksnapshot.cc. A little code duplication has emerged between them, but I think
> this is acceptable.
> 
> On 2011/06/06 15:59:15, Vitaly Repeshko wrote:
> > http://codereview.chromium.org/7066048/diff/7001/samples/shell.cc
> > File samples/shell.cc (right):
> > 
> > http://codereview.chromium.org/7066048/diff/7001/samples/shell.cc#newcode312
> > samples/shell.cc:312: BZip2Decompressor startup_data_decompressor;
> > It seems unfortunate that the shell sample is using something that looks
like
> > internal code to demonstrate the compression feature. Could you document
that
> > it's safe to use its implementation details? Or how about we expose a
> documented
> > convenience API DecompressWithCallback?
> > 
> > http://codereview.chromium.org/7066048/diff/7001/src/bz2-decompress.h
> > File src/bz2-decompress.h (right):
> > 
> >
>
http://codereview.chromium.org/7066048/diff/7001/src/bz2-decompress.h#newcode31
> > src/bz2-decompress.h:31: class BZip2Decompressor {
> > This should be some namespace and should be documented.

Should we add support for compressed startupdata to d8 as well?