847479 - Improve seekable stream (szip) compression and CLI

Status: RESOLVED FIXED

Description

[Mike Hommey [:glandium]]

Comment 1

[Mike Hommey [:glandium]]

Created attachment 720773 [details] [diff] [review]
Add buffer length information when initializing a SeekableZStream

Comment 2

[Mike Hommey [:glandium]]

Created attachment 720775 [details] [diff] [review]
Add a helper class for a buffer mapped from a file

Comment 3

[Mike Hommey [:glandium]]

Created attachment 720776 [details] [diff] [review]
Add a command line argument to szip to decompress an existing seekable stream

Comment 4

[Mike Hommey [:glandium]]

Created attachment 720779 [details] [diff] [review]
Sanity check that szip's compressed output can properly be decompressed

Comment 5

[Mike Hommey [:glandium]]

Created attachment 720781 [details] [diff] [review]
Use raw zlib streams when compressing with szip

And keep compatibility to decompress old streams.

Comment 6

[Mike Hommey [:glandium]]

Created attachment 720782 [details] [diff] [review]
Allow to specify a chunk size on szip command line

Comment 7

[Mike Hommey [:glandium]]

Created attachment 720797 [details] [diff] [review]
Add smart filters borrowed from xz-utils to improve SeekableZStream compression rate

This makes (compressed) libxul.so significantly smaller, and closer to the gzipped size (which is better because szip is compressing by chunks), at the expense of decompression time:
- uncompressed: 21311612
- compressed without filter: 11174496
- compressed with filter: 10752640
- gzip -9: 10604797

Since the format allows the filter to be enabled or not, we can decide later if the trade is worth it.

Gerv, the filter code is borrowed from xz-utils, is the comment enough in mozglue/linker/SeekableZStream.cpp? (the file is MPL2 currently)
The original code is
- http://git.tukaani.org/?p=xz.git;a=blob;f=src/liblzma/simple/arm.c;h=258d870fead34bf32e520f4cdef7132a20a6c610;hb=e7b424d267a34803db8d92a3515528be2ed45abd
- http://git.tukaani.org/?p=xz.git;a=blob;f=src/liblzma/simple/armthumb.c;h=06c21e4067a56e75105177a2c26a2c30c153d791;hb=e7b424d267a34803db8d92a3515528be2ed45abd

Comment 8

[Mike Hommey [:glandium]]

Created attachment 720799 [details] [diff] [review]
Add a dictionary to improve compression rate

The dictionary population is quite stupid, but works well enough.

Combined with filters, this brings us even closer to gzip -9:
- uncompressed: 21311612
- compressed with filter + dict: 10616818
- gzip -9: 10604797

Comment 9

[Gervase Markham [:gerv] (not reading bugmail)]

Comment on attachment 720797 [details] [diff] [review]
Add smart filters borrowed from xz-utils to improve SeekableZStream compression rate

If it's PD, you don't even legally need the comment - although it's good in case there's a bug in the code and it needs porting either way. So this is fine :-) r=gerv.

Gerv

Comment 10

[Mike Hommey [:glandium]]

Created attachment 721235 [details] [diff] [review]
Sanity check that szip's compressed output can properly be decompressed

Comment 11

[Mike Hommey [:glandium]]

Created attachment 721237 [details] [diff] [review]
Allow to specify a chunk size on szip command line

Comment 12

[Mike Hommey [:glandium]]

Created attachment 721239 [details] [diff] [review]
Add a dictionary to improve compression rate

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 720775 [details] [diff] [review]
Add a helper class for a buffer mapped from a file

Review of attachment 720775 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/szip.cpp
@@ +16,5 @@
>  
> +class FileBuffer: public MappedPtr
> +{
> +public:
> +  bool Init(const char *name, bool _writable = false)

Nit: can you make this |writable_| (or similar, I'm not picky about the naming convention) so that it doesn't conflict with the implementation namespace?

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 720776 [details] [diff] [review]
Add a command line argument to szip to decompress an existing seekable stream

Review of attachment 720776 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/szip.cpp
@@ +181,5 @@
> +
> +  struct stat st;
> +  int ret = fstat(origBuf.getFd(), &st);
> +  if (ret == -1) {
> +    log("Couldn't stat %s: %s", firstArg[1], strerror(errno));

I think you want to use firstArg[0] here.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 720781 [details] [diff] [review]
Use raw zlib streams when compressing with szip

Review of attachment 720781 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/SeekableZStream.h
@@ +37,5 @@
>    /* Number of chunks */
>    le_uint32 nChunks;
>  
>    /* Size of last chunk (> 0, <= Chunk size) */
> +  le_uint16 lastChunkSize;

Can you add a check to szip.cpp to ensure that we're always going to assign in-range values to lastChunkSize?

Or, hm, looking at szip.cpp, I guess since CHUNK is 16K, we're assuming that lastChunkSize is always going to be < 16K?  A check asserting that would be good.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 721237 [details] [diff] [review]
Allow to specify a chunk size on szip command line

Review of attachment 721237 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/szip.cpp
@@ +245,5 @@
> +      if (!firstArg[0])
> +        break;
> +      if (!GetSize(firstArg[0], &chunkSize) || !chunkSize ||
> +          (chunkSize % 4096) || (chunkSize > 1 << 15)) {
> +        log("Invalid chunk size");

Aha, here's the check I was looking for. :)  If you could make that 15 more obviously dependent on the fields in the header, that would be good.

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 721239 [details] [diff] [review]
Add a dictionary to improve compression rate

Review of attachment 721239 [details] [diff] [review]:
-----------------------------------------------------------------

I think this looks sane, but I'd appreciate clarification on Dictionary's constructor.

::: mozglue/linker/szip.cpp
@@ +90,5 @@
> +    std::vector<stat_pair> statsVec(stats.begin(), stats.end());
> +    std::sort(statsVec.begin(), statsVec.end(), stat_cmp);
> +
> +    piece *dictPieces = reinterpret_cast<piece *>(
> +                        static_cast<void *>(*this));

This code and the loop below made me scratch my head for a while.  I'm guessing this DTRT because the cast to |void*| returns the mapped buffer, yes?  But AFAICS, those bits haven't been initialized by Buffer's constructor...

@@ +130,4 @@
>    {}
>  
> +  const static signed char winSizeLog = 15;
> +  const static size_t winSize = 1 << 15;

|1 << winSizeLog| ?

Comment 18

[Mike Hommey [:glandium]]

Created attachment 721503 [details] [diff] [review]
Allow to specify a chunk size on szip command line

Re-requesting review for good measure.

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 721503 [details] [diff] [review]
Allow to specify a chunk size on szip command line

Works for me, thanks.

Comment 20

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0effb755bdbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b328fb4f3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2244aa90a7b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2e14d6c705
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a66a2a47f2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/88226153a994
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeba894662b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/777c81f4d4a4

Comment 21

[Ed Morley (Away 17th-19th Aug) [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0effb755bdbe
https://hg.mozilla.org/mozilla-central/rev/c6b328fb4f3e
https://hg.mozilla.org/mozilla-central/rev/2244aa90a7b3
https://hg.mozilla.org/mozilla-central/rev/4b2e14d6c705
https://hg.mozilla.org/mozilla-central/rev/9a66a2a47f2b
https://hg.mozilla.org/mozilla-central/rev/88226153a994
https://hg.mozilla.org/mozilla-central/rev/eeba894662b8
https://hg.mozilla.org/mozilla-central/rev/777c81f4d4a4