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

[Peter Levart]

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