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

Improve encoding performance #81

Merged
merged 3 commits into from
Apr 9, 2020

Conversation

yaauie
Copy link
Contributor

@yaauie yaauie commented Apr 6, 2020

This change-set replaces character-by-character input sanitizers with equivalent gsub expressions, improving performance especially when many fields are referenced.

For example, when used with the TCP Output and simple field values, I was able to achieve the following maximum throughputs on a 16-worker pipeline:

fields char-by-char gsub inline gsub constantized
1 45k 47k 49k
12 18k 28k 30k
100 3.5k 7.5k 8.0k

Copy link
Contributor

@colinsurprenant colinsurprenant left a comment

Choose a reason for hiding this comment

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

LGTM

lib/logstash/codecs/cef.rb Outdated Show resolved Hide resolved
lib/logstash/codecs/cef.rb Outdated Show resolved Hide resolved
@yaauie
Copy link
Contributor Author

yaauie commented Apr 7, 2020

@colinsurprenant can you take one more look? I refactored since your LGTM to get even better performance.

value = value.to_s.gsub(/[^a-zA-Z0-9]/, "")
return value
value.to_s
.gsub(/[^a-zA-Z0-9]/, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

cosmetic: newline might not be necessary here.

@colinsurprenant
Copy link
Contributor

LGTM²

@yaauie yaauie merged commit 2a8edcd into logstash-plugins:master Apr 9, 2020
@yaauie yaauie deleted the encode-performance branch April 9, 2020 01:27
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.

4 participants