Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add option to disable deflate #20

Merged
merged 5 commits into from
Dec 2, 2016
Merged

Conversation

kturney
Copy link

@kturney kturney commented Nov 17, 2016

potential fix for #19

@@ -18,6 +18,7 @@ var graylog = function graylog(config) {
this.client = null;
this.hostname = config.hostname || require('os').hostname();
this.facility = config.facility || 'Node.js';
this.deflate = config.deflate;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

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
Copy link
Author

kturney commented Nov 28, 2016

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
Copy link
Collaborator

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

@stelcheck
Copy link
Member

stelcheck commented Nov 29, 2016

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
Copy link
Collaborator

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
Copy link
Member

stelcheck commented Nov 29, 2016

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
Copy link
Author

kturney commented Nov 29, 2016

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
Copy link
Collaborator

@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
Copy link
Author

kturney commented Nov 30, 2016

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
Copy link
Collaborator

ronkorving commented Nov 30, 2016

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
Copy link
Author

kturney commented Nov 30, 2016

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
Copy link
Collaborator

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
Copy link
Collaborator

LGTM. @stelcheck wanna do a last review?

@stelcheck
Copy link
Member

Lean, mean, and LGTM :shipit:

@stelcheck
Copy link
Member

Also, @kturney thank you for your contribution!
epic_high_five_by_josefinjonsson

@kturney
Copy link
Author

kturney commented Dec 2, 2016

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

@ronkorving ronkorving merged commit 484f622 into Wizcorp:master Dec 2, 2016
@ronkorving
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants