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

[Peter Levart]

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