Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v10]

2021-11-09 Thread Stephen Colebourne
On Wed, 3 Nov 2021 22:55:23 GMT, Claes Redestad  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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v10]

2021-11-03 Thread Claes Redestad
On Wed, 3 Nov 2021 21:57:44 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:
> 
>   Remove accidentally committed experimental @Stable (no effect on micros)

Thanks, Naoto!

-

PR: https://git.openjdk.java.net/jdk/pull/6188


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v10]

2021-11-03 Thread Naoto Sato
On Wed, 3 Nov 2021 21:57:44 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:
> 
>   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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v10]

2021-11-03 Thread Claes Redestad
> 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:

  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=6188=09
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v9]

2021-11-03 Thread Claes Redestad
> 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:

  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=6188=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v8]

2021-11-03 Thread Naoto Sato
On Wed, 3 Nov 2021 19:44:35 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 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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v7]

2021-11-03 Thread Claes Redestad
On Wed, 3 Nov 2021 18:17:38 GMT, Naoto Sato  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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v8]

2021-11-03 Thread Claes Redestad
> 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 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=6188=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v7]

2021-11-03 Thread Naoto Sato
On Wed, 3 Nov 2021 17:23:51 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:
> 
>   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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v7]

2021-11-03 Thread Claes Redestad
On Wed, 3 Nov 2021 17:33:36 GMT, Naoto Sato  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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v7]

2021-11-03 Thread Naoto Sato
On Wed, 3 Nov 2021 17:23:51 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:
> 
>   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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v7]

2021-11-03 Thread Claes Redestad
> 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:

  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=6188=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v6]

2021-11-03 Thread Claes Redestad
On Wed, 3 Nov 2021 14:24:28 GMT, Stephen Colebourne  
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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v6]

2021-11-03 Thread Stephen Colebourne
On Wed, 3 Nov 2021 13:14:42 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 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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]

2021-11-03 Thread Joe Darcy
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]

2021-11-03 Thread Claes Redestad
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 [v6]

2021-11-03 Thread Claes Redestad
> 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 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=6188=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v5]

2021-11-03 Thread Claes Redestad
> 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:

  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=6188=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]

2021-11-03 Thread Claes Redestad
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 [v4]

2021-11-03 Thread Claes Redestad
> 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:

  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=6188=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]

2021-11-03 Thread Claes Redestad
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]

2021-11-03 Thread Claes Redestad
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]

2021-11-03 Thread Stephen Colebourne
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]

2021-11-02 Thread Claes Redestad
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]

2021-11-02 Thread Claes Redestad
> 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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v2]

2021-11-02 Thread Stephen Colebourne
On Mon, 1 Nov 2021 22:35:58 GMT, Claes Redestad  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/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.

-

PR: https://git.openjdk.java.net/jdk/pull/6188


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v2]

2021-11-01 Thread Claes Redestad
On Mon, 1 Nov 2021 22:27:08 GMT, Claes Redestad  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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v2]

2021-11-01 Thread Claes Redestad
> 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:

  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=6188=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter

2021-11-01 Thread Claes Redestad
On Mon, 1 Nov 2021 22:18:52 GMT, Stephen Colebourne  
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
>
> 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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter

2021-11-01 Thread Stephen Colebourne
On Mon, 1 Nov 2021 13:04:20 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

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


RFR: 8276220: Reduce excessive allocations in DateTimeFormatter

2021-11-01 Thread Claes Redestad
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-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

-

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=6188=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


Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter

2021-11-01 Thread Claes Redestad
On Mon, 1 Nov 2021 13:04:20 GMT, Claes Redestad  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-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

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.normHH:mm:ss  thrpt   15  192015.139 ± 
0.352B/op
formatInstants   HH:mm:ss.SSS  thrpt   15   2.772 ± 
0.036  ops/ms
  ·gc.alloc.rate.normHH:mm:ss.SSS  thrpt   15  696805.289 ± 
11280.987B/op
formatInstants  -MM-dd'T'HH:mm:ss  thrpt   15   4.025 ± 
0.056  ops/ms
  ·gc.alloc.rate.norm   -MM-dd'T'HH:mm:ss  thrpt   15  264020.926 ± 
0.555B/op
formatInstants  -MM-dd'T'HH:mm:ss.SSS  thrpt   15   2.121 ± 
0.026  ops/ms
  ·gc.alloc.rate.norm   -MM-dd'T'HH:mm:ss.SSS  thrpt   15  768811.221 ± 
11281.118B/op
formatZonedDateTime  HH:mm:ss  thrpt   15   8.797 ± 
0.254  ops/ms
  ·gc.alloc.rate.normHH:mm:ss  thrpt   15   96007.787 ± 
0.331B/op
formatZonedDateTime  HH:mm:ss.SSS  thrpt   15   3.109 ± 
0.024  ops/ms
  ·gc.alloc.rate.normHH:mm:ss.SSS  thrpt   15  600798.055 ± 
11280.992B/op
formatZonedDateTime -MM-dd'T'HH:mm:ss  thrpt   15   4.814 ± 
0.037  ops/ms
  ·gc.alloc.rate.norm   -MM-dd'T'HH:mm:ss  thrpt   15  168013.636 ± 
0.389B/op
formatZonedDateTime -MM-dd'T'HH:mm:ss.SSS  thrpt   15   2.299 ± 
0.059  ops/ms
  ·gc.alloc.rate.norm   -MM-dd'T'HH:mm:ss.SSS  thrpt   15  680012.566 ± 
11281.140B/op


Patched:

Benchmark   (pattern)   Mode  Cnt   Score  
Error   Units
formatInstants   HH:mm:ss  thrpt   15   7.721 ±
0.114  ops/ms
  ·gc.alloc.rate.normHH:mm:ss  thrpt   15  120010.043 ±
0.934B/op
formatInstants   HH:mm:ss.SSS  thrpt   15   5.684 ±
0.083  ops/ms
  ·gc.alloc.rate.normHH:mm:ss.SSS  thrpt   15  120009.973 ±
0.608B/op
formatInstants  -MM-dd'T'HH:mm:ss  thrpt   15   4.962 ±
0.058  ops/ms
  ·gc.alloc.rate.norm   -MM-dd'T'HH:mm:ss  thrpt   15  120010.027 ±
0.703B/op
formatInstants  -MM-dd'T'HH:mm:ss.SSS  thrpt   15   3.889 ±
0.068  ops/ms
  ·gc.alloc.rate.norm   -MM-dd'T'HH:mm:ss.SSS  thrpt   15  120010.284 ±
1.003B/op
formatZonedDateTime  HH:mm:ss  thrpt   15  11.087 ±
0.404  ops/ms
  ·gc.alloc.rate.normHH:mm:ss  thrpt   15   24002.072 ±
0.219B/op
formatZonedDateTime  HH:mm:ss.SSS  thrpt   15   7.325 ±
0.139  ops/ms
  ·gc.alloc.rate.normHH:mm:ss.SSS  thrpt   15   24002.080 ±
0.267B/op
formatZonedDateTime -MM-dd'T'HH:mm:ss  thrpt   15   6.127 ±
0.040  ops/ms
  ·gc.alloc.rate.norm   -MM-dd'T'HH:mm:ss  thrpt   15   24002.084 ±
0.459B/op
formatZonedDateTime -MM-dd'T'HH:mm:ss.SSS  thrpt   15   4.576 ±
0.049  ops/ms
  ·gc.alloc.rate.norm   -MM-dd'T'HH:mm:ss.SSS  thrpt   15   24002.102 ±
0.511B/op


The most dramatic improvement is seen for `formatZonedDateTime` using the 
format `-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