Debian lz4

[fabbione]

No description provided.

[fabbione]

Side note: CI will pass happily because lz4 build is not enabled yet for debian builders. I tested this one on debian-unstable-x86-64 builder manually. Once the patch is merged, I´ll enable lz4 in CI.

[fabbione]

Side note: CI will pass happily because lz4 build is not enabled yet for debian builders. I tested this one on debian-unstable-x86-64 builder manually. Once the patch is merged, I´ll enable lz4 in CI.

[wferi]

I think the lz4hc.h include could go.

[wferi]

This comment is a little confusing, do you set 9 as default or as maximum? Or is it that knet defaults to the maximum allowed by the compressor?
Also, 0 means "use default value" for LZ4, but "no compression" for zlib (-1 selects some default there), which knet does not allow. Do we need consistency here?

[fabbione]

On 2/2/2018 1:26 PM, wferi wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In libknet/compress_lz4.c <#128 (comment)>: > @@ -10,6 +10,7 @@ #include "config.h" #include <errno.h> +#include <lz4.h> #include <lz4hc.h> I think the |lz4hc.h| include could go.

No it can´t go. In recent versions of lz4, lz4.h does not include lz4hc.h and we need the decompressor.

------------------------------------------------------------------------ In libknet/compress_lz4hc.c <#128 (comment)>: > @@ -22,8 +23,13 @@ #define KNET_LZ4HC_MAX LZ4HC_MAX_CLEVEL #endif #ifndef KNET_LZ4HC_MAX -#define KNET_LZ4HC_MAX 0 -#error Please check lz4hc.h for missing LZ4HC_CLEVEL_MAX or LZ4HC_MAX_CLEVEL variants +/* + * older releases of lz4 do not define LZ4HC_CLEVEL range. + * According to lz4hc.h, any value between 0 and 16 is valid. + * We defalt to 9 based on the comments in the include file + * from older versions. + */ +#define KNET_LZ4HC_MAX 9 This comment is a little confusing, do you set 9 as default or as maximum? Or is it that knet defaults to the maximum allowed by the compressor?

default. If you place a value higher than default, you only get a warning. according to lz4 docs, any value >= MAX are == MAX.

Also, 0 means "use default value" for LZ4, but "no compression" for zlib (-1 selects some default there), which knet does not allow. Do we need consistency here?

hmmmm this might be tricky because when I first read zlib doc and tested, it refused values < 0. we might end up rabbit holing into each library compression version/configuration combo to validate those values. Suggestions? As for the "no compress" I don´t think we should allow it. Most of those libraries will add at least a memcpy / memmove on no-compress that adds unnecessary latency to knet. If you don´t want compress, don´t pick one. Fabio

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#128 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAW6rbiNB3mZcpaOnhz1QRgi9tCdLraeks5tQv73gaJpZM4R2u8d>.

[fabbione]

Ok pushed a change to default to 16 that is the max allowed in older versions.

[wferi]

"Fabio M. Di Nitto" <notifications@github.com> writes:

Ok pushed a change to default to 16 that is the max allowed in older versions.

I think that's fair. Newer versions will specify their maxima and everybody is happy. -- Feri

[wferi]

"Fabio M. Di Nitto" <notifications@github.com> writes:

On 2/2/2018 1:26 PM, wferi wrote: > ***@***.**** commented on this pull request. > > ------------------------------------------------------------------------ > > In libknet/compress_lz4.c > <#128 (comment)>: > > > @@ -10,6 +10,7 @@ > #include "config.h" > > #include <errno.h> > +#include <lz4.h> > #include <lz4hc.h> > > I think the |lz4hc.h| include could go. No it can´t go. In recent versions of lz4, lz4.h does not include lz4hc.h and we need the decompressor.

I mean from libknet/compress_lz4.c, not from libknet/compress_lz4hc.c.

> ------------------------------------------------------------------------ > > In libknet/compress_lz4hc.c > <#128 (comment)>: > > > @@ -22,8 +23,13 @@ > #define KNET_LZ4HC_MAX LZ4HC_MAX_CLEVEL > #endif > #ifndef KNET_LZ4HC_MAX > -#define KNET_LZ4HC_MAX 0 > -#error Please check lz4hc.h for missing LZ4HC_CLEVEL_MAX or LZ4HC_MAX_CLEVEL variants > +/* > + * older releases of lz4 do not define LZ4HC_CLEVEL range. > + * According to lz4hc.h, any value between 0 and 16 is valid. > + * We defalt to 9 based on the comments in the include file > + * from older versions. > + */ > +#define KNET_LZ4HC_MAX 9 > > This comment is a little confusing, do you set 9 as default or as > maximum? Or is it that knet defaults to the maximum allowed by the > compressor? default. If you place a value higher than default, you only get a warning. according to lz4 docs, any value >= MAX are == MAX.

Yeah, but it's name is KNET_LZ4HC_MAX for a reason: knet won't allow higher values.

> Also, 0 means "use default value" for LZ4, but "no compression" for > zlib (-1 selects some default there), which knet does not allow. Do we > need consistency here? hmmmm this might be tricky because when I first read zlib doc and tested, it refused values < 0. we might end up rabbit holing into each library compression version/configuration combo to validate those values. Suggestions?

If all compression plugins admit a "level" described by an int, we can simply pass it through. After all, the person selecting a compressor is responsible for configuring that compressor. This also means knet can't simply use any generic default value, it must be prescribed for each compressor separately.

As for the "no compress" I don´t think we should allow it. Most of those libraries will add at least a memcpy / memmove on no-compress that adds unnecessary latency to knet. If you don´t want compress, don´t pick one.

Everything but "no compress" should be enough for everyone. :) I think we should avoid getting into the way even when we can't see the point of a given configuration. Unless it means unreasonable maintenance burden, which doesn't seem to be the case here. In other words, I'd rather avoid patronizing the user. -- Feri

[fabbione]

I start to hate github.. can´t reply via email and didn´t get notification of the previous comment.

(manual quoting)

====

No it can´t go. In recent versions of lz4, lz4.h does not include
lz4hc.h and we need the decompressor.
I mean from libknet/compress_lz4.c, not from libknet/compress_lz4hc.c.

ack, yes this should be able to go.

===

Yeah, but it's name is KNET_LZ4HC_MAX for a reason: knet won't allow
higher values.

The name might be misleading, but it does allow values higher than MAX. It will print a warning than the value is remapped:

    if (compress_level > KNET_LZ4HC_MAX) {
            log_warn(knet_h, KNET_SUB_LZ4HCCOMP, "lz4hc installed on this system supports up to compression level %d. Higher values behaves as %d", KNET_LZ4HC_MAX, KNET_LZ4HC_MAX);
    }

but does not fail.

===

If all compression plugins admit a "level" described by an int, we can
simply pass it through. After all, the person selecting a compressor is
responsible for configuring that compressor. This also means knet can't
simply use any generic default value, it must be prescribed for each
compressor separately.

the reason why I started validating values was lzo2. It has some really odd values to map and without some proper logging, it´s hard for a user to figure it out. Perhaps we can just warn better in lzo2 and free the others. What do you think?

Tho I agree, we could potentially drop all those checks and pass-through.

[fabbione]

Feri, I changed the header inclusion for compress_lz4.c.

As for dropping all the compress level checks, I would like to do that in another PR and talk a bit with @chrissie-c too on the subject.

does that work for you?

[wferi]

Objections withdrawn. :)

[fabbione]

rebased on top of master. Unless you get to ack it again, i´ll use my super powers later today.

[knet-ci-bot]

re-approving after rebase on top of master. No code changes.

[fabbione]

lz4 build in debian is now enable in CI on all builders.