RFR: JDK-8074003 java.time.zone.ZoneRules.getOffset(java.time.Instant) can be optimized

[Stephen Colebourne]

The approach works for me, and the patch is valid as is.
Stephen


On 30 April 2015 at 11:24, Peter Levart <peter.levart at gmail.com> wrote:
>
> On 04/29/2015 05:35 PM, Roger Riggs wrote:
>>
>> Hi Peter,
>>
>> There should be two changesets; so pretend the truncation has been
>> performed for this change.
>> It maybe useful to backport the performance improvement to jdk 8 but the
>> spec change
>> will have to be in 9 (or wait for a maintenance release).
>>
>
> Hi Roger,
>
> So perhaps it would be best to push what we have in webrev.03 now, so that
> it can be backported to 8u directly without modifications and simplify
> equals/compareTo/getInstant as part of the changeset for 8079063. I think
> this is more consistent. And I can prepare the change for 8079063 right away
> so the spec change process can be started.
>
> Do I have a go for webrev.03 for jdk9 ?
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransition.epochSecond/webrev.03/
>
> Regards, Peter
>
>
>> The simplification of toInstant can happen with the changeset for 8079063.
>>
>> Thanks, Roger
>>
>>
>> On 4/29/2015 11:26 AM, Peter Levart wrote:
>>>
>>>
>>>
>>> On 04/29/2015 03:31 PM, Roger Riggs wrote:
>>>>
>>>> Hi Peter,
>>>>
>>>> Point taken about the constructor and that should have a spec
>>>> clarification to ignore the nanoseconds.
>>>> The issue is tracked with:
>>>> JDK-8079063 <https://bugs.openjdk.java.net/browse/JDK-8079063>
>>>> ZoneOffsetTransition constructor should ignore nanoseconds
>>>>
>>>> With that, the compareTo method can be simpler.  The rest looks fine.
>>>>
>>>> Roger
>>>
>>>
>>> Hi Roger,
>>>
>>> Should I prepare a patch for both issues in one changeset as the correct
>>> compareTo/equals depends on the truncation or should I just pretend that
>>> truncation has been performed and make this change accordingly or should I
>>> 1st do the JDK-8079063 and then this one on top?
>>>
>>> Also, getInstant() can be much simpler if the truncation is performed:
>>> return Instant.of(epochSecond);
>>>
>>> Regards, Peter
>>>
>>>>
>>>>
>>>> On 4/29/2015 5:33 AM, Peter Levart wrote:
>>>>>
>>>>>
>>>>> On 04/27/2015 06:51 PM, Stephen Colebourne wrote:
>>>>>>
>>>>>> One additional change is needed. The compareTo() method can rely on
>>>>>> the new epochSecond field as well.
>>>>>> Otherwise good!
>>>>>> Stephen
>>>>>
>>>>>
>>>>> Hi Stephen,
>>>>>
>>>>> LocalDateTime (transition) has nanosecond precision. It may be that
>>>>> transitions loaded from file in ZoneRules only have second precisions, but
>>>>> ZoneOffsetTransition is a public class with public factory method that takes
>>>>> a LocalDateTime transition parameter, so I think compareTo() can't rely on
>>>>> epochSecond alone. But epochSecond can be used as optimization in
>>>>> compareTo() as well as equals():
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransition.epochSecond/webrev.03/
>>>>>
>>>>> An alternative to keeping epochSecond field in ZoneOffsetTransition
>>>>> would be to keep a reference to Instant instead. Instant contains an
>>>>> epochSecond field (as well as nanos) and could be used for both
>>>>> toEpochSecond() and getInstant() methods.
>>>>>
>>>>> What do you think?
>>>>>
>>>>> It also occurred to me that serialization format of
>>>>> ZoneOffsetTransition is not adequate currently as it looses nanosecond
>>>>> precision.
>>>>>
>>>>> Regards, Peter
>>>>>
>>>>>>
>>>>>> On 27 April 2015 at 17:24, Peter Levart <peter.levart at gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi again,
>>>>>>>
>>>>>>> Here's another optimization to be reviewed that has been discussed a
>>>>>>> while
>>>>>>> ago (just rebased from webrev.01) and approved by Stephen:
>>>>>>>
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransition.epochSecond/webrev.02/
>>>>>>>
>>>>>>>
>>>>>>> The discussion about it is intermingled with the
>>>>>>> ZoneId.systemDefault()
>>>>>>> discussion and starts about here:
>>>>>>>
>>>>>>>
>>>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-February/031873.html
>>>>>>>
>>>>>>>
>>>>>>> The rationale for the optimization is speeding-up the conversion from
>>>>>>> epoch
>>>>>>> time to LocalDateTime. This conversion uses
>>>>>>> ZoneRules.getOffset(Instant)
>>>>>>> where there is a loop over ZoneOffsetTransition[] array that searches
>>>>>>> for
>>>>>>> 1st transition that has its toEpochSecond value less than the
>>>>>>> Instant's
>>>>>>> epochSecond. This calls ZoneOffsetTransition.toEpochSecond
>>>>>>> repeatedly,
>>>>>>> converting ZoneOffsetTransition.transition which is a LocalDateTime
>>>>>>> to
>>>>>>> epochSecond. This repeated conversion is unnecessary, as
>>>>>>> ZoneOffsetTransition[] array is part of ZoneRules which is cached.
>>>>>>> Optimizing the ZoneOffsetTransition implementation (keeping both
>>>>>>> LocalDateTime variant and eposhSecond variant of transition time as
>>>>>>> the
>>>>>>> object's state) speeds up this conversion.
>>>>>>>
>>>>>>>
>>>>>>> Regards, Peter
>>>>>>>
>>>>>
>>>>
>>>
>>
>