Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Wed, 3 Nov 2021 12:17:09 GMT, Claes Redestad 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
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Wed, 3 Nov 2021 12:44:39 GMT, Claes Redestad 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/src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java#L3543). > 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
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Wed, 3 Nov 2021 12:21:00 GMT, Claes Redestad 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/src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java#L3543).
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
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Wed, 3 Nov 2021 11:53:52 GMT, Stephen Colebourne
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
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Wed, 3 Nov 2021 12:04:10 GMT, Stephen Colebourne 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
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Tue, 2 Nov 2021 11:03:02 GMT, Claes Redestad 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-S`).
>>
>> 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
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
On Tue, 2 Nov 2021 07:31:09 GMT, Stephen Colebourne 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/time/temporal/TemporalAccessor.html#getLong(java.time.temporal.TemporalField)) > 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
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]
> 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-S`). > > 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=6188=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=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
