Re: RFR: 8283681: Improve ZonedDateTime offset handling [v2]
On Fri, 25 Mar 2022 15:17:02 GMT, Stephen Colebourne
wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add nanoOfSecond parameter, make micro less reliant on constants
>
> src/java.base/share/classes/java/time/ZoneOffset.java line 512:
>
>> 510: @Override
>> 511: /* package-private */ ZoneOffset getOffset(long epochSecond, int
>> nanoOfSecond) {
>> 512: return this;
>
> An alternate approach would be for `ZoneOffset` to cache the instance of
> `ZoneRules` either on construction or first use (racy idiom would be OK).
> That way this issue is solved for the many different places that call
> `zoneId.getRules()`.
Why not both? I measured an improvement using that alone, but specifically
avoiding going via getRules is faster still (without adversely affecting
`ZoneRegion` paths). I've added the cache in `ZoneOffset`, along with an
override of `ZoneId::normalized` in `ZoneOffset` to shortcut the `getRules`.
-
PR: https://git.openjdk.java.net/jdk/pull/7957
Re: RFR: 8283681: Improve ZonedDateTime offset handling [v2]
On Fri, 25 Mar 2022 14:35:46 GMT, Claes Redestad wrote:
>> Richard Startin prompted me to have a look at a case where java.time
>> underperforms relative to joda time
>> (https://twitter.com/richardstartin/status/1506975932271190017).
>>
>> It seems the java.time test of his suffer from heavy allocations due
>> ZoneOffset::getRules allocating a new ZoneRules object every time and escape
>> analysis failing to do the thing in his test. The patch here adds a simple
>> specialization so that when creating ZonedDateTimes using a ZoneOffset we
>> don't query the rules at all. This removes the risk of extra allocations and
>> slightly speeds up ZonedDateTime creation for both ZoneOffset (+14%) and
>> ZoneRegion (+5%) even when EA works like it should (the case in the here
>> provided microbenchmark).
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add nanoOfSecond parameter, make micro less reliant on constants
src/java.base/share/classes/java/time/ZoneOffset.java line 512:
> 510: @Override
> 511: /* package-private */ ZoneOffset getOffset(long epochSecond, int
> nanoOfSecond) {
> 512: return this;
An alternate approach would be for `ZoneOffset` to cache the instance of
`ZoneRules` either on construction or first use (racy idiom would be OK). That
way this issue is solved for the many different places that call
`zoneId.getRules()`.
-
PR: https://git.openjdk.java.net/jdk/pull/7957
Re: RFR: 8283681: Improve ZonedDateTime offset handling [v2]
On Fri, 25 Mar 2022 14:52:09 GMT, Roger Riggs wrote: >> Done. >> >> Sadly it seems the smaller improvement I got on >> `getYearFromMillisZoneRegion/-UTC` was due avoiding the added arithmetic in >> `Instant.ofEpochSecond(sec, nanos)`: >> >> Benchmark Mode Cnt Score Error >> Units >> GetYearBench.getYearFromMillisZoneOffset thrpt 15 27.579 ? 0.030 >> ops/ms >> GetYearBench.getYearFromMillisZoneRegion thrpt 15 9.570 ? 0.091 >> ops/ms >> GetYearBench.getYearFromMillisZoneRegionUTC thrpt 15 28.063 ? 0.030 >> ops/ms >> >> Benchmark Mode Cnt Score Error >> Units >> GetYearBench.getYearFromMillisZoneOffset thrpt 15 34.791 ? 0.030 >> ops/ms >> GetYearBench.getYearFromMillisZoneRegion thrpt 15 9.526 ? 0.122 >> ops/ms >> GetYearBench.getYearFromMillisZoneRegionUTC thrpt 15 28.056 ? 0.040 >> ops/ms >> >> >> `getYearFromMillisZoneOffset` is still good. > > I would expect that `nanoAdjustment` is zero in most cases, would it hurt > performance to test for zero and skip the math? Actually I think it might be fairly common with a `nanoAdjustment` (e.g. timestamps with milliseconds), so not sure such a test is profitable. But I think it was correct to omit the nano parts for the `ZonedDateTime` constructor, since it's validating that the `nanoOfSecond` parameter is in the range 0-9. I'll revert the change to add the 2nd parameter to the new, internal getOffset method. - PR: https://git.openjdk.java.net/jdk/pull/7957
Re: RFR: 8283681: Improve ZonedDateTime offset handling [v2]
On Fri, 25 Mar 2022 14:28:41 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/time/ZoneRegion.java line 183:
>>
>>> 181: @Override
>>> 182: /* package-private */ ZoneOffset getOffset(long epochSecond) {
>>> 183: return
>>> getRules().getOffset(Instant.ofEpochSecond(epochSecond));
>>
>> The nanoAdjustment passed to `ofEpochSecond` is discarded in this code.
>> It may be larger than a second; see `Instant.ofEpochSecond(sec, nano)` for
>> the details.
>> Adding a second parameter to the `getOffset` method could be the remedy.
>
> Done.
>
> Sadly it seems the smaller improvement I got on
> `getYearFromMillisZoneRegion/-UTC` was due avoiding the added arithmetic in
> `Instant.ofEpochSecond(sec, nanos)`:
>
> Benchmark Mode Cnt Score Error
> Units
> GetYearBench.getYearFromMillisZoneOffset thrpt 15 27.579 ? 0.030
> ops/ms
> GetYearBench.getYearFromMillisZoneRegion thrpt 15 9.570 ? 0.091
> ops/ms
> GetYearBench.getYearFromMillisZoneRegionUTC thrpt 15 28.063 ? 0.030
> ops/ms
>
> Benchmark Mode Cnt Score Error
> Units
> GetYearBench.getYearFromMillisZoneOffset thrpt 15 34.791 ? 0.030
> ops/ms
> GetYearBench.getYearFromMillisZoneRegion thrpt 15 9.526 ? 0.122
> ops/ms
> GetYearBench.getYearFromMillisZoneRegionUTC thrpt 15 28.056 ? 0.040
> ops/ms
>
>
> `getYearFromMillisZoneOffset` is still good.
I would expect that `nanoAdjustment` is zero in most cases, would it hurt
performance to test for zero and skip the math?
-
PR: https://git.openjdk.java.net/jdk/pull/7957
Re: RFR: 8283681: Improve ZonedDateTime offset handling [v2]
On Fri, 25 Mar 2022 12:33:21 GMT, Richard Startin wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add nanoOfSecond parameter, make micro less reliant on constants > > test/micro/org/openjdk/bench/java/time/GetYearBench.java line 70: > >> 68: private static final long[] INSTANT_MILLIS = createInstants(); >> 69: >> 70: private static final int[] YEARS = new int[INSTANT_MILLIS.length]; > > Does it make any difference if these aren't constant? Interestingly a slight increase in the measured gain (14% -> 25%). I think we should favor non-constant data to subdue irrelevant JIT shenanigans so I pushed the changes. - PR: https://git.openjdk.java.net/jdk/pull/7957
Re: RFR: 8283681: Improve ZonedDateTime offset handling [v2]
> Richard Startin prompted me to have a look at a case where java.time > underperforms relative to joda time > (https://twitter.com/richardstartin/status/1506975932271190017). > > It seems the java.time test of his suffer from heavy allocations due > ZoneOffset::getRules allocating a new ZoneRules object every time and escape > analysis failing to do the thing in his test. The patch here adds a simple > specialization so that when creating ZonedDateTimes using a ZoneOffset we > don't query the rules at all. This removes the risk of extra allocations and > slightly speeds up ZonedDateTime creation for both ZoneOffset (+14%) and > ZoneRegion (+5%) even when EA works like it should (the case in the here > provided microbenchmark). Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add nanoOfSecond parameter, make micro less reliant on constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/7957/files - new: https://git.openjdk.java.net/jdk/pull/7957/files/faf35394..355f192a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7957&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7957&range=00-01 Stats: 15 lines in 5 files changed: 2 ins; 0 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/7957.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7957/head:pull/7957 PR: https://git.openjdk.java.net/jdk/pull/7957
Re: RFR: 8283681: Improve ZonedDateTime offset handling [v2]
On Fri, 25 Mar 2022 14:09:03 GMT, Roger Riggs wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add nanoOfSecond parameter, make micro less reliant on constants
>
> src/java.base/share/classes/java/time/ZoneRegion.java line 183:
>
>> 181: @Override
>> 182: /* package-private */ ZoneOffset getOffset(long epochSecond) {
>> 183: return getRules().getOffset(Instant.ofEpochSecond(epochSecond));
>
> The nanoAdjustment passed to `ofEpochSecond` is discarded in this code.
> It may be larger than a second; see `Instant.ofEpochSecond(sec, nano)` for
> the details.
> Adding a second parameter to the `getOffset` method could be the remedy.
Done.
Sadly it seems the smaller improvement I got on
`getYearFromMillisZoneRegion/-UTC` was due avoiding the added arithmetic in
`Instant.ofEpochSecond(sec, nanos)`:
Benchmark Mode Cnt Score Error Units
GetYearBench.getYearFromMillisZoneOffset thrpt 15 27.579 ? 0.030 ops/ms
GetYearBench.getYearFromMillisZoneRegion thrpt 15 9.570 ? 0.091 ops/ms
GetYearBench.getYearFromMillisZoneRegionUTC thrpt 15 28.063 ? 0.030 ops/ms
Benchmark Mode Cnt Score Error Units
GetYearBench.getYearFromMillisZoneOffset thrpt 15 34.791 ? 0.030 ops/ms
GetYearBench.getYearFromMillisZoneRegion thrpt 15 9.526 ? 0.122 ops/ms
GetYearBench.getYearFromMillisZoneRegionUTC thrpt 15 28.056 ? 0.040 ops/ms
`getYearFromMillisZoneOffset` is still good.
-
PR: https://git.openjdk.java.net/jdk/pull/7957
