Closed Bug 960367 Opened 10 years ago Closed 10 years ago

OdinMonkey: compress source stored in in cache file

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file, 1 obsolete file)

This is super easy and the compression ratio means 4-8x savings on 10-200MB files (the latter are unoptimized, unminified debug builds).
Attached patch compress (WIP) (obsolete) — Splinter Review
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.
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)
Do you have a resulting build at hand?
Flags: needinfo?(mh+mozilla)
(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?
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.
Depends on: 960860
Attached patch compressSplinter Review
Attachment #8361471 - Flags: review?(sstangl)
Attachment #8360851 - Attachment is obsolete: true
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+
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
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.