The approach works for me, and the patch is valid as is. Stephen
On 30 April 2015 at 11:24, Peter Levart <peter.lev...@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.lev...@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 >>>>>>> >>>>> >>>> >>> >> >