Closed
Bug 960367
Opened 10 years ago
Closed 10 years ago
OdinMonkey: compress source stored in in cache file
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(1 file, 1 obsolete file)
8.93 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
This is super easy and the compression ratio means 4-8x savings on 10-200MB files (the latter are unoptimized, unminified debug builds).
Assignee | ||
Comment 1•10 years ago
|
||
Here's the patch, which uses LZ4. I still need to do some timing measurements to compare this to zlib. What's strange is that it works in the shell but crashes, somewhere in the dynamic loader, when trying to call into LZ4::compress.
Assignee | ||
Comment 2•10 years ago
|
||
glandium, as a knower of builds and linkages and things, do you know what could be causing this crash? This is on x64 Ubuntu, for reference.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #3) I do indeed. Anything you want me to test or do you just want me to email you it in a zip?
Assignee | ||
Comment 5•10 years ago
|
||
Mike found the offender: http://hg.mozilla.org/mozilla-central/annotate/bcbe93f41547/modules/zlib/src/mozzconf.h #define compress ... in a header #included by jsutil.h. Mike said he'd file a bug about us not doing this, but it seems like we should *also* avoid #including zlib.h from jsutil.h which is #included from, ya know, everywhere.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8361471 -
Flags: review?(sstangl)
Assignee | ||
Updated•10 years ago
|
Attachment #8360851 -
Attachment is obsolete: true
Comment 7•10 years ago
|
||
Comment on attachment 8361471 [details] [diff] [review] compress Review of attachment 8361471 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/AsmJSModule.cpp @@ +842,5 @@ > + > + // Compress the chars in preparation for storage: > + > + uncompressedSize_ = (endOffset(parser) - beginOffset(parser)) * sizeof(jschar); > + if (!compressedBuffer_.resize(LZ4::maxCompressedSize(uncompressedSize_))) maxCompressedSize() can fail by overflow. Its implementation currently MOZ_ASSERT()s that this cannot happen, but in a release, a specially-crafted AsmJS payload would be able to cause allocation overflow, at which point LZ4 would compress into unallocated memory. A check of the compressed size versus uncompressedSize_ would be sufficient. mfbt/Compression.h:37 states "Worst case size evaluation is provided by function LZ4_compressBound()", but that function is in lz4.h, not part of the mfbt interface. Would you mind changing that to say "maxCompressedSize()" instead? @@ +849,5 @@ > + const jschar *chars = parser.tokenStream.rawBase() + beginOffset(parser); > + const char *source = reinterpret_cast<const char*>(chars); > + size_t compressedSize = LZ4::compress(source, uncompressedSize_, compressedBuffer_.begin()); > + if (compressedSize > UINT32_MAX) > + return false; LZ4_compress() can also return 0 in case of failure per mfbt/lz4.h:64. It uses malloc() internally.
Attachment #8361471 -
Flags: review?(sstangl) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Nice catches! A maxCompressedSize() overflow seems strictly theoretical (as it would require a script taking up ~3.9GB), but it seems cleaner to guard as you suggest. https://hg.mozilla.org/integration/mozilla-inbound/rev/a0a1851773db
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a0a1851773db
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•