8276220: Reduce excessive allocations in DateTimeFormatter #6188
Conversation
|
Microbenchmark numbers for the supplied Baseline: Patched: The most dramatic improvement is seen for |
|
👋 Welcome back redestad! A progress list of the required criteria for merging this PR into |
Webrevs
|
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
Outdated
Show resolved
Hide resolved
| private final boolean decimalPoint; | ||
|
|
||
| // only instantiated and used if there's ever a value outside the allowed range | ||
| private FractionPrinterParser fallback; |
There was a problem hiding this comment.
This class has to be safe in a multi-threaded environment. I'm not convinced it is safe right now, as the usage doesn't follow the standard racy single check idiom. At a minimum, there needs to be a comment explaining the thread-safety issues.
There was a problem hiding this comment.
Yes, I'll make sure to read into a local variable.
| range.checkValidValue(value, field); | ||
| BigDecimal minBD = BigDecimal.valueOf(range.getMinimum()); | ||
| BigDecimal rangeBD = BigDecimal.valueOf(range.getMaximum()).subtract(minBD).add(BigDecimal.ONE); | ||
| field.range().checkValidValue(value, field); | ||
| BigDecimal valueBD = BigDecimal.valueOf(value).subtract(minBD); | ||
| BigDecimal fraction = valueBD.divide(rangeBD, 9, RoundingMode.FLOOR); | ||
| // stripTrailingZeros bug |
There was a problem hiding this comment.
I believe this bug was fixed a while back, so this code could be simplified.
There was a problem hiding this comment.
Got a reference to which bug this was and how it manifests?
There was a problem hiding this comment.
If you're referring to JDK-6480539: "BigDecimal.stripTrailingZeros() has no effect on zero itself ("0.0")", it was fixed in JDK 8.
| ValueRange range = field.range(); | ||
| range.checkValidValue(value, field); | ||
| BigDecimal minBD = BigDecimal.valueOf(range.getMinimum()); | ||
| BigDecimal rangeBD = BigDecimal.valueOf(range.getMaximum()).subtract(minBD).add(BigDecimal.ONE); |
There was a problem hiding this comment.
I wouldn't be surprised if there is a way to replace the use of BigDecimal with calculations using long. Fundamentally, calculations like 15/60 -> 0.25 are not hard, but it depends on whether the exact results can be matched across a wide range of possible inputs.
There was a problem hiding this comment.
I think the main engineering challenge is writing tests that ensure that we don't lose precision on an arbitrary fractional field.
| // [0-999999999], we can't assume that holds for any custom Temporal | ||
| if (!field.range().isValidIntValue(value)) { | ||
| if (fallback == null) { | ||
| fallback = new FractionPrinterParser(field, minWidth, maxWidth, decimalPoint, subsequentWidth); |
There was a problem hiding this comment.
It would be nice to see a test case cover this.
There was a problem hiding this comment.
When adding a test for this I discovered that FractionPrinterParser::format will end up calling field.range().checkValidValue(value, field) here. This means that the pre-existing implementation does check the value range and throws exceptions when trying to print a value outside of the field range.
To mimic the existing behavior we have to do the same check in NanosPrinterParser::format and drop the fallback (which would have somewhat nonsensical output for values outside the range, anyhow).
There was a problem hiding this comment.
Added a test case showing that values that are outside the range throw DateTimeException. This passes with and without the patch and mainly documents the pre-existing behavior.
…r should do the same. Simplify accordingly.
There was a problem hiding this comment.
Thanks for adding the new tests, and finding that fraction formatting is more constrained than I thought it was.
I think there is a case for a spec update in DateTimeFormatterBuilder to clarify the constraint on the input value (at this point, that seems better than changing the behaviour of fraction formatting). As things stand, the exception that is thrown is not described by the spec. (The spec change could be part of this PR or a separate one).
|
Thanks for reviewing, @jodastephen! I think a spec update sounds good, but I think that should be done in as a follow-up. If you would be willing to provide the wording for such a spec update I can take care of creating a CSR. |
|
Looks good. I'd create a new test case class out of |
It was only indirectly a test of I realized though that with my changes the test coverage of |
| @@ -79,7 +79,7 @@ | |||
| /** | |||
| * Test FractionPrinterParser. | |||
There was a problem hiding this comment.
OK, then I'd add some comments that the test covers NanoPrinterParser as well.
|
Oh, just noticed the copyright year->2021 in modified files. Should have noticed before. |
|
@cl4es This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 42 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Thanks, Naoto! /integrate |
|
Going to push as commit ce8c767.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
|
@cl4es For to |
Prompted by a request from Volkan Yazıcı I took a look at why the java.time formatters are less efficient for some common patterns than custom formatters in apache-commons and log4j. This patch reduces the gap, without having looked at the third party implementations.
When printing times:
Strings before appending them to the bufferappendFractionforNANO_OF_SECONDto avoid use ofBigDecimalThis means a speed-up and reduction in allocations when formatting almost any date or time pattern, and especially so when including sub-second parts (
S-SSSSSSSSS).Much of the remaining overhead can be traced to the need to create a
DateTimePrintContextand adjustingInstants into aZonedDateTimeinternally. We could likely also win performance by specializing some common patterns.Testing: tier1-3
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188$ git checkout pull/6188Update a local copy of the PR:
$ git checkout pull/6188$ git pull https://git.openjdk.java.net/jdk pull/6188/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 6188View PR using the GUI difftool:
$ git pr show -t 6188Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6188.diff