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

[Peter Levart]

Stephen, Roger,

Thanks for reviews. This has been pushed to jdk9-dev.

Regards, Peter

On 04/30/2015 02:47 PM, Roger Riggs wrote:
> 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
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>