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

[Roger Riggs]

Hi Peter,

Yes, go ahead with the patch as is.

Thanks, Roger


On 4/30/2015 6:24 AM, Peter Levart 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
>>>>>>>
>>>>>
>>>>
>>>
>>
>