-
Notifications
You must be signed in to change notification settings - Fork 408
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
FastISOTimestampFormatter as a fast alternative to Java's DateTimeFormatter for standard ISO formats #711
Conversation
…eTimeFormatter for standard ISO formats The FastISOTimestampFormatter is a fast alternative to Java's DateTimeFormatter for the most commonly used ISO formats, i.e.: - ISO_OFFSET_DATE_TIME - ISO_ZONED_DATE_TIME - ISO_LOCAL_DATE_TIME - ISO_DATE_TIME - ISO_INSTANT Instead of computing every date/time fields everytime it is invoked like the standard DateTimeFormatter does, the fast formatter remembers the previous formatted value and uses it to replace only the second and millis parts if the next timestamp to format is within the same minute.
76b1b44
to
3dc67c7
Compare
Codecov Report
@@ Coverage Diff @@
## main #711 +/- ##
============================================
+ Coverage 68.87% 69.42% +0.54%
- Complexity 1190 1206 +16
============================================
Files 160 161 +1
Lines 4623 4719 +96
Branches 458 481 +23
============================================
+ Hits 3184 3276 +92
+ Misses 1192 1190 -2
- Partials 247 253 +6
Continue to review full report at Codecov.
|
…TimestampFormatter Use a format not supported by the FastISOTimestampFormatter to force the test to go through the reflection code.
src/main/java/net/logstash/logback/composite/FastISOTimestampFormatter.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/composite/FastISOTimestampFormatter.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/composite/FastISOTimestampFormatter.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/composite/FastISOTimestampFormatter.java
Show resolved
Hide resolved
src/main/java/net/logstash/logback/composite/FastISOTimestampFormatter.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/composite/FastISOTimestampFormatter.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/composite/FastISOTimestampFormatter.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/composite/FastISOTimestampFormatter.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/composite/AbstractFormattedTimestampJsonProvider.java
Show resolved
Hide resolved
src/test/java/net/logstash/logback/composite/FastISOTimestampFormatterTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the suggested changes.
Approved as-is. Just had some comments on minor stuff that I'm not terribly concerned about.
@philsttr Code always looks obvious to the writer after writing... but 6 months later is another story ;-) So your comments are definitely worth it! I have another question tho. The "fast" formatter is used only when the Isn't this confusing for the user? When a pattern is used instead of a format name, shouldn't we try to detect it matches one of the supported formats? This may not be easy tho: what would be the pattern for ISO_INSTANT? The "fast" formatter can support any format as long as the prefix and suffix parts are stable during one minute, i.e. they are not affected by the second and millis time field. Do you think we should make it more general so it can support arbitrary patterns as long as they match this rule? |
Your choice. I imagine most people are probably using one of the standards already, so I'm not too concerned about supporting arbitrary formats. But it's your call. |
The FastISOTimestampFormatter is a fast alternative to Java's DateTimeFormatter for the most commonly used ISO formats, i.e.:
Instead of computing every date/time fields every time it is invoked like the standard DateTimeFormatter does, the fast formatter remembers the previous formatted value and uses it to replace only the second and millis parts if the next timestamp to format is within the same minute.
This new formatter is currently used only when the
FormattedTimestampJsonProvider
is configured with one of default format listed above (expressed between[]
in the configuration). A standard DateTimeFormatter is used for the other standard formats or if a custom pattern is configured (even if the pattern matches one of the supported formats). We can extend support for other formats if needed...To highlight the benefits I ran a quick benchmark whose only job is to format
System.currentTimeMillis()
as fast as possible. This test is of course not representative of any real world scenario but it gives a fair overview of the differences between the two approaches.As we can see, the "fast" formatter is 7x faster and produces 10x less garbage.
I then ran the "LogstashEncoder" benchmark to see how it performs with the new fast formatter. This benchmark invokes the LogstashEncoder as fast as it can during 3 rounds of 10 seconds each. I also ran it against LLE 7.01 and 6.6:
We can see that
7.1-SNAPSHOT
is nearly 50% faster than7.0.1
and produces 50% less garbage.7.0.1
and6.6
have roughly the same throughput but7.0.1
produces less memory allocation. This is due to the reuse of the Jackson JsonFactory introduced in 7.0.Pending things todo: