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. http://codereview.chromium.org/7066048/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
