robur-coop / albatross

Albatross: orchestrate and manage MirageOS unikernels with Solo5
ISC License
142 stars 17 forks source link

Upgrade albatross to decompress.1.3.0 #71

Closed dinosaure closed 3 years ago

dinosaure commented 3 years ago

Minor update to decompress.1.3.0. The format does not change and the compression ratio is better (see https://github.com/mirage/decompress/pull/108).

hannesm commented 3 years ago

Thanks! Two questions:

dinosaure commented 3 years ago

Since 38f5085 the ?level (in Vmm_compress.compress) is no longer forwarded to Zl.Higher.compress -- is there a reason for this? Since that function has a ?level parameter, it'd be good to pass it on :)

Before decompress.1.3.0, ?level did not mean anything but now, it has an impact on ratio, speed and memory usage yes 👍 so I agree that we should pass it now. An other point is the interval. Before, ?level was between 0 and 3. Now, it's between 0 and 9 (inclusive).

In uncompress the De.make_window is fine, only in compress we need the De.Lz77.make_window?

Yes, internally the "window" used for the inflation is not the same for the deflation. For the compression, the window is 2 times longer to be able to lookup matches. To be sure to use a longer window, I update the type required by the compression.

hannesm commented 3 years ago

@dinosaure thanks. in albatross, I pass on the level, and use 9 for remote connections (client/albatross_client_bistro.ml) and 0 (no compression for local deployment - client/albatross_client_local.ml):

from client/albatross_cli.ml

let compress_level default =
  let doc = "Compression level (0 for no compression, 1-3 fixed with static huffman, 4-9 dynamic with canonic huffman)" in
  Arg.(value & opt int default & info [ "compression-level" ] ~doc)

and further on:

  match compression with
    | 0 -> Cstruct.of_string image, false
    | level ->
      let img = Vmm_compress.compress ~level image in
      Cstruct.of_string img, true

is the docstring and the code fine? :)

dinosaure commented 3 years ago

Yes, but I just discover that between De and Zl (so DEFLATE and zlib), the level is not passed correctly (it's a bug). So, I would like to say to not pass (not yet) the level. I will fix on my side this bug.

dinosaure commented 3 years ago

Yes, but I just discover that between De and Zl (so DEFLATE and zlib), the level is not passed correctly (it's a bug). So, I would like to say to not pass (not yet) the level. I will fix on my side this bug.

Ah no, we don't have a bug (it's my mistake). So the documentation is good, only one point, decompress does not handle the 0 no-compression. It compress at any level. Then. the static huffman is not available by level but by the ?dynamic argument (default to [true]). The documentation does not mention that (and I should improve such detail).

So it's more accurate that more the level is higher, more the ratio is better.

hannesm commented 3 years ago

thanks