-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Use garbage-free formatter for s
and S
patterns
#3338
Conversation
This PR improves #3139, by introducing a new `InstantPatternFormatter` for patterns of the form "ss\.S{n}". Unlike the previous formatter based on `DateTimeFormatter`, the formatter is garbage-free. We also simplify the merging algorithm for pattern formatter factories, by moving the merging logic to the pattern formatter factories themselves. This PR does not contain a separate change log entry, since #3139 has not been published yet. Fixes #3337.
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.
I like the changes, they are to-the-point and restores the old garbage-free date & time formatting without involving [too much] manual computation. 💯 I would really really appreciate 🥺 if we can restore the old
PatternSequence
name (IMHO, it greatly lowers the cognitive barrier compared to overloading the overloaded formatter concept.)- Method (i.e.,
tryMerge()
) documentation with examples
Additionally,
- Mind sharing the before-after results for
InstantPatternFormatterBenchmark
andInstantPatternFormatterBenchmark
, please? - Manual needs to be updated on parts where subminute precision is stated to be not garbage-free
Allow me to remind that the long-term maintainability (readability & simplicity) of this particular piece of code is of uttermost importance. We implemented several date & time formatters each having its own quirks because we could not dare to fix/extend the old one.
.../org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java
Show resolved
Hide resolved
.../org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java
Show resolved
Hide resolved
.../org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java
Outdated
Show resolved
Hide resolved
...java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java
Show resolved
Hide resolved
...java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java
Outdated
Show resolved
Hide resolved
...java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java
Outdated
Show resolved
Hide resolved
...java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java
Outdated
Show resolved
Hide resolved
...java/org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatter.java
Outdated
Show resolved
Hide resolved
.../org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java
Outdated
Show resolved
Hide resolved
.../org/apache/logging/log4j/core/util/internal/instant/InstantPatternDynamicFormatterTest.java
Show resolved
Hide resolved
Co-authored-by: Volkan Yazıcı <volkan@yazi.ci>
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.
Left some minor comments, overall code changes LGTM. 💯 IMHO, we should still
- Share benchmark results
- Update
pattern-layout.adoc
(on garbage-free aspects of%d
)
We assume that timezones can change in the middle of a (UTC) hour, so they have a precision of minutes.
Here are the results of the
Each operation formats 1000 timestamps that differ less than a second between each other. Reference
Where Before
After
I wouldn't attach too much importance to the precise numbers, since Running the benchmark on a single thread for the two best performing implementations gives:
Note that |
In 0b198fc, I optimized performance a little bit, by:
Overall this gives a performance around 60% higher on the ISO8601 format:
It also doubles the single-thread performance (around 23.1 kops/ms). |
@ppkarwasz, a734365 invalidated |
This PR improves #3139, by introducing a new
InstantPatternFormatter
for patterns of the form "ss.S{n}". Unlike the previous formatter based onDateTimeFormatter
, the formatter is garbage-free.We also simplify the merging algorithm for pattern formatter factories, by moving the merging logic to the pattern formatter factories themselves.
This PR does not contain a separate change log entry, since #3139 has not been published yet.
Fixes #3337.