RFR: 8276220: Reduce excessive allocations in DateTimeFormatter
Prompted by a request from Volkan Yazıcı I took a look at why DataTimeFormatters are much less efficient for some common patterns than custom formatters in apache-commons and log4j. This patch address some of that gap, without having looked at the third party implementations. When printing times: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal` This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns. Testing: tier1-3 ------------- Commit messages: - 8276220: Reduce excessive allocations in DateTimeFormatter Changes: https://git.openjdk.java.net/jdk/pull/6188/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276220 Stats: 429 lines in 4 files changed: 407 ins; 10 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
On Mon, 1 Nov 2021 13:04:20 GMT, Claes Redestad <redestad@openjdk.org> wrote:
Prompted by a request from Volkan Yazıcı I took a look at why DataTimeFormatters are much less efficient for some common patterns than custom formatters in apache-commons and log4j. This patch address some of that gap, without having looked at the third party implementations.
When printing times: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Microbenchmark numbers for the supplied `DateTimeFormatterBench`. Baseline: Benchmark (pattern) Mode Cnt Score Error Units formatInstants HH:mm:ss thrpt 15 6.558 ± 0.125 ops/ms ·gc.alloc.rate.norm HH:mm:ss thrpt 15 192015.139 ± 0.352 B/op formatInstants HH:mm:ss.SSS thrpt 15 2.772 ± 0.036 ops/ms ·gc.alloc.rate.norm HH:mm:ss.SSS thrpt 15 696805.289 ± 11280.987 B/op formatInstants yyyy-MM-dd'T'HH:mm:ss thrpt 15 4.025 ± 0.056 ops/ms ·gc.alloc.rate.norm yyyy-MM-dd'T'HH:mm:ss thrpt 15 264020.926 ± 0.555 B/op formatInstants yyyy-MM-dd'T'HH:mm:ss.SSS thrpt 15 2.121 ± 0.026 ops/ms ·gc.alloc.rate.norm yyyy-MM-dd'T'HH:mm:ss.SSS thrpt 15 768811.221 ± 11281.118 B/op formatZonedDateTime HH:mm:ss thrpt 15 8.797 ± 0.254 ops/ms ·gc.alloc.rate.norm HH:mm:ss thrpt 15 96007.787 ± 0.331 B/op formatZonedDateTime HH:mm:ss.SSS thrpt 15 3.109 ± 0.024 ops/ms ·gc.alloc.rate.norm HH:mm:ss.SSS thrpt 15 600798.055 ± 11280.992 B/op formatZonedDateTime yyyy-MM-dd'T'HH:mm:ss thrpt 15 4.814 ± 0.037 ops/ms ·gc.alloc.rate.norm yyyy-MM-dd'T'HH:mm:ss thrpt 15 168013.636 ± 0.389 B/op formatZonedDateTime yyyy-MM-dd'T'HH:mm:ss.SSS thrpt 15 2.299 ± 0.059 ops/ms ·gc.alloc.rate.norm yyyy-MM-dd'T'HH:mm:ss.SSS thrpt 15 680012.566 ± 11281.140 B/op Patched: Benchmark (pattern) Mode Cnt Score Error Units formatInstants HH:mm:ss thrpt 15 7.721 ± 0.114 ops/ms ·gc.alloc.rate.norm HH:mm:ss thrpt 15 120010.043 ± 0.934 B/op formatInstants HH:mm:ss.SSS thrpt 15 5.684 ± 0.083 ops/ms ·gc.alloc.rate.norm HH:mm:ss.SSS thrpt 15 120009.973 ± 0.608 B/op formatInstants yyyy-MM-dd'T'HH:mm:ss thrpt 15 4.962 ± 0.058 ops/ms ·gc.alloc.rate.norm yyyy-MM-dd'T'HH:mm:ss thrpt 15 120010.027 ± 0.703 B/op formatInstants yyyy-MM-dd'T'HH:mm:ss.SSS thrpt 15 3.889 ± 0.068 ops/ms ·gc.alloc.rate.norm yyyy-MM-dd'T'HH:mm:ss.SSS thrpt 15 120010.284 ± 1.003 B/op formatZonedDateTime HH:mm:ss thrpt 15 11.087 ± 0.404 ops/ms ·gc.alloc.rate.norm HH:mm:ss thrpt 15 24002.072 ± 0.219 B/op formatZonedDateTime HH:mm:ss.SSS thrpt 15 7.325 ± 0.139 ops/ms ·gc.alloc.rate.norm HH:mm:ss.SSS thrpt 15 24002.080 ± 0.267 B/op formatZonedDateTime yyyy-MM-dd'T'HH:mm:ss thrpt 15 6.127 ± 0.040 ops/ms ·gc.alloc.rate.norm yyyy-MM-dd'T'HH:mm:ss thrpt 15 24002.084 ± 0.459 B/op formatZonedDateTime yyyy-MM-dd'T'HH:mm:ss.SSS thrpt 15 4.576 ± 0.049 ops/ms ·gc.alloc.rate.norm yyyy-MM-dd'T'HH:mm:ss.SSS thrpt 15 24002.102 ± 0.511 B/op The most dramatic improvement is seen for `formatZonedDateTime` using the format `yyyy-MM-dd'T'HH:mm:ss.SSS`, which sees a 2x speed-up and almost 97% reduction in allocations. The same variant using `Instant`s see a 1.8x speed-up and almost 85% reduction in allocations. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Mon, 1 Nov 2021 13:04:20 GMT, Claes Redestad <redestad@openjdk.org> wrote:
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Changes requested by scolebourne (Author). src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3269:
3267: return false; 3268: } 3269: int val = value.intValue(); // NANO_OF_SECOND must fit in an int and can't be negative
Unfortunately, this is not a valid assumption, and it affects the logic of the optimization more generally (methods where non-negative is assumed). Basically, NANO_OF_SECOND (and all other fields in the formatter) can have any `long` value. Despite immediate appearances, the value is not limited to 0 to 999,999,999. This is because you are allowed to create an implementation of `Temporal` that returns values outside that range. No such class exists in the JDK, but such a class would be perfectly legal. As a related example, it would be perfectly value to write a time class that ran from 03:00 to 26:59 each day, thus HOUROF_DAY cannot be assumed by the formatter to be between 0 and 23. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Mon, 1 Nov 2021 22:18:52 GMT, Stephen Colebourne <scolebourne@openjdk.org> wrote:
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3269:
3267: return false; 3268: } 3269: int val = value.intValue(); // NANO_OF_SECOND must fit in an int and can't be negative
Unfortunately, this is not a valid assumption, and it affects the logic of the optimization more generally (methods where non-negative is assumed).
Basically, NANO_OF_SECOND (and all other fields in the formatter) can have any `long` value. Despite immediate appearances, the value is not limited to 0 to 999,999,999. This is because you are allowed to create an implementation of `Temporal` that returns values outside that range. No such class exists in the JDK, but such a class would be perfectly legal. As a related example, it would be perfectly value to write a time class that ran from 03:00 to 26:59 each day, thus HOUROF_DAY cannot be assumed by the formatter to be between 0 and 23.
The commentary on this line could probably be improved, but this is in a private printer-parser that will only be used for NANO_OF_SECOND and not any arbitrary `Temporal` (see line 704), thus I fail to see how this assumption can fail (since NANO_OF_SECOND specifies a value range from 0 to 999,999,999). I considered writing a more generic integral-fraction printer parser that would optimize for any value-range that fits in an int, but seeing how NANO_OF_SECOND is likely the only one used in practice and with a high demand for better efficiency I opted to specialize for it more directly. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Mon, 1 Nov 2021 22:27:08 GMT, Claes Redestad <redestad@openjdk.org> wrote:
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3269:
3267: return false; 3268: } 3269: int val = value.intValue(); // NANO_OF_SECOND must fit in an int and can't be negative
Unfortunately, this is not a valid assumption, and it affects the logic of the optimization more generally (methods where non-negative is assumed).
Basically, NANO_OF_SECOND (and all other fields in the formatter) can have any `long` value. Despite immediate appearances, the value is not limited to 0 to 999,999,999. This is because you are allowed to create an implementation of `Temporal` that returns values outside that range. No such class exists in the JDK, but such a class would be perfectly legal. As a related example, it would be perfectly value to write a time class that ran from 03:00 to 26:59 each day, thus HOUROF_DAY cannot be assumed by the formatter to be between 0 and 23.
The commentary on this line could probably be improved, but this is in a private printer-parser that will only be used for NANO_OF_SECOND and not any arbitrary `TemporalField` (see line 704), thus I fail to see how this assumption can fail (since NANO_OF_SECOND specifies a value range from 0 to 999,999,999).
I considered writing a more generic integral-fraction printer parser that would optimize for any value-range that fits in an int, but seeing how NANO_OF_SECOND is likely the only one used in practice and with a high demand for better efficiency I opted to specialize for it more directly.
I see what you're saying that an arbitrary `Temporal` could define its own fields with its own ranges, but I would consider it a design bug if such an implementation at a whim redefines the value ranges of well-defined constants such as `ChronoField.NANO_OF_SECOND` or `HOUR_OF_DAY`. I'd expect such a `Temporal` would have to define its own enumeration of allowed `TemporalField`s. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Mon, 1 Nov 2021 22:35:58 GMT, Claes Redestad <redestad@openjdk.org> wrote:
The commentary on this line could probably be improved, but this is in a private printer-parser that will only be used for NANO_OF_SECOND and not any arbitrary `TemporalField` (see line 704), thus I fail to see how this assumption can fail (since NANO_OF_SECOND specifies a value range from 0 to 999,999,999).
I considered writing a more generic integral-fraction printer parser that would optimize for any value-range that fits in an int, but seeing how NANO_OF_SECOND is likely the only one used in practice and with a high demand for better efficiency I opted to specialize for it more directly.
I see what you're saying that an arbitrary `Temporal` could define its own fields with its own ranges, but I would consider it a design bug if such an implementation at a whim redefines the value ranges of well-defined constants such as `ChronoField.NANO_OF_SECOND` or `HOUR_OF_DAY`. I'd expect such a `Temporal` would have to define its own enumeration of allowed `TemporalField`s.
That isn't the design model however. The design model for the formatter is a `Map` like view of field to value. Any value may be associated with any field - that is exactly what `Temporal` offers. [`TempralAccessor.getLong()`](https://download.java.net/java/early_access/loom/docs/api/java.base/java/tim...)) is very explicit about this. As indicated above, the positive part is that an hour-of-day of 26 can be printed by a user-written `WrappingLocalTime` class. The downside is the inability to make optimizing assumptions as per this code. FWIW, I had originally intended to write dedicated private formatters where the pattern and type to be formatted are known, such as `LocalDate` and the ISO pattern. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Tue, 2 Nov 2021 07:31:09 GMT, Stephen Colebourne <scolebourne@openjdk.org> wrote:
I see what you're saying that an arbitrary `Temporal` could define its own fields with its own ranges, but I would consider it a design bug if such an implementation at a whim redefines the value ranges of well-defined constants such as `ChronoField.NANO_OF_SECOND` or `HOUR_OF_DAY`. I'd expect such a `Temporal` would have to define its own enumeration of allowed `TemporalField`s.
That isn't the design model however. The design model for the formatter is a `Map` like view of field to value. Any value may be associated with any field - that is exactly what `Temporal` offers. [`TempralAccessor.getLong()`](https://download.java.net/java/early_access/loom/docs/api/java.base/java/tim...)) is very explicit about this.
As indicated above, the positive part is that an hour-of-day of 26 can be printed by a user-written `WrappingLocalTime` class. The downside is the inability to make optimizing assumptions as per this code.
FWIW, I had originally intended to write dedicated private formatters where the pattern and type to be formatted are known, such as `LocalDate` and the ISO pattern.
Ok, I've added a fallback to instantiate and use an instance of `FractionPrinterParser` when the value is outside of the range. This has a negligible negative effect on performance in the provided micro-benchmarks. Does this resolve your concerns? I think dedicated private formatters is still a good idea, but I wanted to take a stab (like this) at improving common patterns in the shared code first. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove stray method ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/9e97b4dc..ee13139a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=00-01 Stats: 10 lines in 1 file changed: 0 ins; 10 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add fallback for values outside the allowable range ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/ee13139a..21092323 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=01-02 Stats: 12 lines in 1 file changed: 11 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
On Tue, 2 Nov 2021 11:03:02 GMT, Claes Redestad <redestad@openjdk.org> wrote:
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Add fallback for values outside the allowable range
Changes requested by scolebourne (Author). src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3158:
3156: 3157: // only instantiated and used if there's ever a value outside the allowed range 3158: private FractionPrinterParser fallback;
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. src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3266:
3264: if (!field.range().isValidIntValue(value)) { 3265: if (fallback == null) { 3266: fallback = new FractionPrinterParser(field, minWidth, maxWidth, decimalPoint, subsequentWidth);
It would be nice to see a test case cover this. src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3290:
3288: range.checkValidValue(value, field); 3289: BigDecimal minBD = BigDecimal.valueOf(range.getMinimum()); 3290: BigDecimal rangeBD = BigDecimal.valueOf(range.getMaximum()).subtract(minBD).add(BigDecimal.ONE);
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. src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3544:
3542: BigDecimal valueBD = BigDecimal.valueOf(value).subtract(minBD); 3543: BigDecimal fraction = valueBD.divide(rangeBD, 9, RoundingMode.FLOOR); 3544: // stripTrailingZeros bug
I believe this bug was fixed a while back, so this code could be simplified. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 12:04:10 GMT, Stephen Colebourne <scolebourne@openjdk.org> wrote:
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Add fallback for values outside the allowable range
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3544:
3542: BigDecimal valueBD = BigDecimal.valueOf(value).subtract(minBD); 3543: BigDecimal fraction = valueBD.divide(rangeBD, 9, RoundingMode.FLOOR); 3544: // stripTrailingZeros bug
I believe this bug was fixed a while back, so this code could be simplified.
Got a reference to which bug this was and how it manifests? ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 12:17:09 GMT, Claes Redestad <redestad@openjdk.org> wrote:
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3544:
3542: BigDecimal valueBD = BigDecimal.valueOf(value).subtract(minBD); 3543: BigDecimal fraction = valueBD.divide(rangeBD, 9, RoundingMode.FLOOR); 3544: // stripTrailingZeros bug
I believe this bug was fixed a while back, so this code could be simplified.
Got a reference to which bug this was and how it manifests?
If you're referring to JDK-6480539: "BigDecimal.stripTrailingZeros() has no effect on zero itself ("0.0")", it was fixed in JDK 8. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 11:53:52 GMT, Stephen Colebourne <scolebourne@openjdk.org> wrote:
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Add fallback for values outside the allowable range
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3158:
3156: 3157: // only instantiated and used if there's ever a value outside the allowed range 3158: private FractionPrinterParser fallback;
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.
Yes, I'll make sure to read into a local variable.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3266:
3264: if (!field.range().isValidIntValue(value)) { 3265: if (fallback == null) { 3266: fallback = new FractionPrinterParser(field, minWidth, maxWidth, decimalPoint, subsequentWidth);
It would be nice to see a test case cover this.
I'll see to it.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3290:
3288: range.checkValidValue(value, field); 3289: BigDecimal minBD = BigDecimal.valueOf(range.getMinimum()); 3290: BigDecimal rangeBD = BigDecimal.valueOf(range.getMaximum()).subtract(minBD).add(BigDecimal.ONE);
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.
I think the main engineering challenge is writing tests that ensure that we don't lose precision on an arbitrary fractional field. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 12:21:00 GMT, Claes Redestad <redestad@openjdk.org> wrote:
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 3266:
3264: if (!field.range().isValidIntValue(value)) { 3265: if (fallback == null) { 3266: fallback = new FractionPrinterParser(field, minWidth, maxWidth, decimalPoint, subsequentWidth);
It would be nice to see a test case cover this.
I'll see to it.
When adding a test for this I discovered that `FractionPrinterParser::format` will end up calling `field.range().checkValidValue(value, field)` [here](https://github.com/openjdk/jdk/blob/579b2c017f24f2266abefd35c2b8f28fa7268d93...). 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). ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 12:44:39 GMT, Claes Redestad <redestad@openjdk.org> wrote:
I'll see to it.
When adding a test for this I discovered that `FractionPrinterParser::format` will end up calling `field.range().checkValidValue(value, field)` [here](https://github.com/openjdk/jdk/blob/579b2c017f24f2266abefd35c2b8f28fa7268d93...). 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).
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. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Single-check idiom ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/21092323..579b2c01 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=02-03 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: FractionPrinterParser checks values to be in range; NanosPrinterParser should do the same. Simplify accordingly. ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/579b2c01..f887c0d7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=03-04 Stats: 15 lines in 2 files changed: 0 ins; 14 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add test verifying OOB values throw exception ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/f887c0d7..1d21a1a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=04-05 Stats: 18 lines in 1 file changed: 18 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 13:14:42 GMT, Claes Redestad <redestad@openjdk.org> wrote:
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Add test verifying OOB values throw exception
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). ------------- Marked as reviewed by scolebourne (Author). PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 14:24:28 GMT, Stephen Colebourne <scolebourne@openjdk.org> wrote:
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Add test verifying OOB values throw exception
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. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Minor cleanup ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/1d21a1a5..3ce6d977 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=05-06 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 17:23:51 GMT, Claes Redestad <redestad@openjdk.org> wrote:
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Minor cleanup
Looks good. I'd create a new test case class out of `TestFractionPrinterParser`, as you introduced the new `NanosPrinterParser`. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 17:33:36 GMT, Naoto Sato <naoto@openjdk.org> wrote:
Looks good. I'd create a new test case class out of `TestFractionPrinterParser`, as you introduced the new `NanosPrinterParser`.
It was only indirectly a test of `FractionPrinterParser`; it's really a test of `PrinterParsers` built using `appendFraction`, which can be either `FractionPrinterParser` or the new `NanosPrinterParser`. So the name is still somewhat appropriate. We could rename it, but splitting it apart seems excessive. I realized though that with my changes the test coverage of `FractionPrinterParser` is substantially reduced, since most of the testing is done using `NANO_OF_SECOND`. I'm adding a set of tests using similar input for `MICRO_OF_SECOND` that will exercise `FractionPrinterParser`. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 17:23:51 GMT, Claes Redestad <redestad@openjdk.org> wrote:
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Minor cleanup
test/jdk/java/time/test/java/time/format/TestFractionPrinterParser.java line 80:
78: 79: /** 80: * Test FractionPrinterParser.
OK, then I'd add some comments that the test covers `NanoPrinterParser` as well. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 18:17:38 GMT, Naoto Sato <naoto@openjdk.org> wrote:
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Minor cleanup
test/jdk/java/time/test/java/time/format/TestFractionPrinterParser.java line 80:
78: 79: /** 80: * Test FractionPrinterParser.
OK, then I'd add some comments that the test covers `NanoPrinterParser` as well.
Ok, done. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add MICRO_OF_SECOND tests to retain proper coverage of FractionPrinterParser ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/3ce6d977..01fce436 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=06-07 Stats: 107 lines in 2 files changed: 103 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 19:44:35 GMT, Claes Redestad <redestad@openjdk.org> wrote:
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Add MICRO_OF_SECOND tests to retain proper coverage of FractionPrinterParser
Oh, just noticed the copyright year->2021 in modified files. Should have noticed before. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Copyrights ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/01fce436..f6adb5b5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=07-08 Stats: 25 lines in 4 files changed: 22 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove accidentally committed experimental @Stable (no effect on micros) ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/f6adb5b5..b663fe63 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6188&range=08-09 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 21:57:44 GMT, Claes Redestad <redestad@openjdk.org> wrote:
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Remove accidentally committed experimental @Stable (no effect on micros)
Looks good. Thank you for the fix! ------------- Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 21:57:44 GMT, Claes Redestad <redestad@openjdk.org> wrote:
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Remove accidentally committed experimental @Stable (no effect on micros)
Thanks, Naoto! ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Wed, 3 Nov 2021 22:55:23 GMT, Claes Redestad <redestad@openjdk.org> wrote:
Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
Remove accidentally committed experimental @Stable (no effect on micros)
Thanks, Naoto!
@cl4es For `DateTimeFormatterBuilder ` spec, I propose changing the existing wording slightly If the field value in the date-time to be printed is invalid it cannot be printed and an exception will be thrown. to If the field value in the date-time to be printed is outside the range of valid values then an exception will be thrown. ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
On Mon, 1 Nov 2021 13:04:20 GMT, Claes Redestad <redestad@openjdk.org> wrote:
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: - Avoid turning integral values into `String`s before appending them to the buffer - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of `BigDecimal`
This 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 `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` internally. We could likely also win performance by specializing some common patterns.
Testing: tier1-3
This pull request has now been integrated. Changeset: ce8c7670 Author: Claes Redestad <redestad@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/ce8c76700ba854f43c48d32b068b30e7d78d... Stats: 564 lines in 4 files changed: 533 ins; 12 del; 19 mod 8276220: Reduce excessive allocations in DateTimeFormatter Reviewed-by: scolebourne, naoto ------------- PR: https://git.openjdk.java.net/jdk/pull/6188
participants (4)
-
Claes Redestad
-
Joe Darcy
-
Naoto Sato
-
Stephen Colebourne