#7676 Don't use cache for Event sprintf, use efficient implementation instead #7692
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Your PR description has lots of nice details, but your commit doesn't have this same information. Can you copy the PR description into your commit message? |
|
@jordansissel sure, sec :) |
|
@jordansissel done :) |
jordansissel
suggested changes
Jul 20, 2017
|
|
||
| if(pos <= template.length() - 1) { | ||
| compiledTemplate.add(new StaticNode(template.substring(pos))); | ||
| if (template.regionMatches(open + 2, "+%s", 0, 3)) { |
There was a problem hiding this comment.
Hardcoding 3 for length gives you interesting results that are differnt from Logstash 5.5. I think you need to check the whole length (close - open - 1 ?)
| outcome | input | v5.5.0 | this pr |
|---|---|---|---|
| same | +%s | 1500508897 | 1500509113 |
| different | +%ss | %43 | 1500509162 |
| different | +%s d y | %30 20 2017 | 1500509234 |
In all cases above, since the first 3 chars are "%+s" it assumes unix epoch time and ignores the rest.
There was a problem hiding this comment.
@jordansissel huh good find, let me a add a test + fix for this case :)
…stash.StringInterpolation#evaluate` by:
* Don't use regex to find the contents between `%{}`, it's way too inefficient for a simple case like this with all its allocations
* Flatten logic out of the many helper classes
* Use a reusable, thread-local `StringBuilder` to serialize into
* Don't use `.substring` for comparisons
* `static` `ObjectMapper`
* `org.joda.time.format.DateTimeFormat#forPattern` not being cached is not an issue, since it's cached in `org.joda.time.format.DateTimeFormat#cPatternCache`
* Added benchmark
* ~15% faster for a few cases I ran, even for repeated patterns with interpolation + couldn't find any case even with date formatting, where this implementation is slower than the cached one
* no leaking on dynamic patterns
|
@jordansissel fixed and test for the problematic case added :) |
elasticsearch-bot
pushed a commit
that referenced
this pull request
Jul 25, 2017
…tringInterpolation#evaluate` by:
* Don't use regex to find the contents between `%{}`, it's way too inefficient for a simple case like this with all its allocations
* Flatten logic out of the many helper classes
* Use a reusable, thread-local `StringBuilder` to serialize into
* Don't use `.substring` for comparisons
* `static` `ObjectMapper`
* `org.joda.time.format.DateTimeFormat#forPattern` not being cached is not an issue, since it's cached in `org.joda.time.format.DateTimeFormat#cPatternCache`
* Added benchmark
* ~15% faster for a few cases I ran, even for repeated patterns with interpolation + couldn't find any case even with date formatting, where this implementation is slower than the cached one
* no leaking on dynamic patterns
Fixes #7692
|
@jordansissel thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7676 and generally improves the performance of
org.logstash.StringInterpolation#evaluateby:Don't use regex to find the contents between
%{}, it's way too inefficient for a simple case like this with all its allocationsFlatten logic out of the many helper classes
Use a reusable, thread-local
StringBuilderto serialize intoDon't use
.substringfor comparisonsstaticObjectMapperorg.joda.time.format.DateTimeFormat#forPatternnot being cached is not an issue, since it's cached inorg.joda.time.format.DateTimeFormat#cPatternCacheAdded benchmark
PS: All tests still pass and this thing has good coverage it seems :P