add option to disable deflate

[kturney]

potential fix for #19

[ronkorving]

Could we force this to a boolean here? Perhaps this.deflate = config.delate !== false; or something? Then the code below doesn't have to do an explicit check against false and the default behavior is defined here instead of on-send.

[ronkorving]

Thanks, just left a small comment.

Also, I'm wondering if it would be wise to detect when deflate is turned on, if we need to deflate at all. That is, if it wouldn't shrink the message to fewer chunks. If the entire message is already 1 chunk, there's little point in deflating, right?

[kturney]

Hi @ronkorving.

I've switched to forcing deflate to a boolean in the constructor, as you commented.

I also liked your idea of only compressing when it will reduce the number of chunks, so I added that check as well.

Thanks for taking a look.

[ronkorving]

Beautiful! cc @stelcheck What do you think? I'm good with this :)

[stelcheck]

As far as the code itself is concerned, I see no issues. But I am not sure this really fixes anything. According to the related node issue (nodejs/node#8871):

There is no real memory leak, it's ordinary - but rather catastrophic - memory fragmentation.

So my understanding is disabling zlib shouldn't be necessary based on memleak issues. But I may be misunderstanding how memory fragmentation will cause issues (which I think we should try to get a proper understanding of).

As for deflating if we can fit all the data in one frame, that does make sense. The one issue I see with this is that now, we have two code paths instead of one. Whether this will result in a tangible performance gain that will offset that fact, I don't know, but if it does, it would be nice to have tangible numbers.

At any rate, I would say it make sense to make zlib optional by configuration (wouldn't even mind to make the default disabled, actually); the fact that GELF makes this optional should be all the reason we need. I would however be cautious about getting smart about when we enable or disable zlib based on optimisation. Since it is based on use-cases, I think that making it configurable should be all that is needed, and it will make the behaviour of the library more predictable (which is rather important whenever a lib really is just an integration layer with some other part of your middleware IMO).

[ronkorving]

That's an interesting perspective. For the sake of this PR I've run some benchmarks and came to the conclusion that not compressing a message that fits in a single UDP packet anyway, gives about a 3x throughput improvement. That seems quite relevant.

I can see why that is not necessarily a case to always turn off compression though, since lack of compression can mean more packets (which can also be slow, I haven't tested this yet), and thus more data loss over UDP. But my conclusion for now is that this single-packet optimization (which in my case is quite common) is going to pay off.

[stelcheck]

Edit: null or undefined, I suppose?

Excellent. In which case, how about this: make it optimized by default, but have the option to completely turn compression on or off?

zlib: null // use default, optimize whenever you can/should
zlib: true // always compress
zlib: false // never compress

This way, as a library user (and likely as a system operator which will end up configuring the application where this lib will be used), you remain in control; if you have a weird bug, you can make the behaviour of the system more predictable through configuration.

[kturney]

so something like

if (this.deflate || (this.deflate !== false && payload.length > this._bufferSize)) {
  zlib.deflate(payload, sendPayload);
} else {
  sendPayload(null, payload);
}

with no boolean coercion in the constructor?

[ronkorving]

@kturney I think that's exactly what @stelcheck is proposing. What are your thoughts?

Just a small nit:

Rather than:

(this.deflate !== false && payload.length > this._bufferSize)

perhaps this is more semantical (while achieving the same):

(this.deflate === null && payload.length > this._bufferSize)

[kturney]

I think it makes sense to enable not deflating single buffer payloads by default.

I also think it makes sense to allow the user to say always or never deflate.

The one thing that feels weird to me is the deflate option boolean effectively becoming a ternary option of true, false, or default, but maybe that's fine.

My reasoning for using this.deflate !== false was so that any falsy value that is not explicitly false is treated as wanting the default behavior.

[ronkorving]

I agree with a boolean being used for what is basically an enum of 3 items is a bit weird. Perhaps we can come up with something better? Eg. strings: always, never, optimal?

[kturney]

So I implemented it as the string enums for the deflate option. It reads better, but there is no real enforcement that the dev actually passed a valid option value. Do we want to add some constraints? Right now it will fall back to the current behavior of always deflating if the deflate option is not never or optimal.

[ronkorving]

I think some assertion at setup time would be good yeah. If people make a typo, it should break instead of behave somewhat differently (and they would never realize it).

[ronkorving]

LGTM. @stelcheck wanna do a last review?

[stelcheck]

Lean, mean, and LGTM

[stelcheck]

Also, @kturney thank you for your contribution!

[kturney]

My pleasure! Thank y'all for taking a look.

[ronkorving]

Thanks!