RFR: JDK-8074003 java.time.zone.ZoneRules.getOffset(java.time.Instant) can be optimized
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.epochSecon... 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.ht... 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
One additional change is needed. The compareTo() method can rely on the new epochSecond field as well. Otherwise good! Stephen On 27 April 2015 at 17:24, Peter Levart <peter.levart@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.epochSecon...
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.ht...
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
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.epochSecon... 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@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.epochSecon...
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.ht...
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
On the LocalDateTime being passed in with nanoseconds, that was an unconsidered use case. The whole offset system relies on second based offsets, so it should really be validated/truncated to remove nanos. That way the equals/compareTo could be simplified again. Seems like a separate issue, but perhaps could be tackled here. You need an Oracle sponsor to tell you ;-) Stephen On 29 April 2015 at 10:33, Peter Levart <peter.levart@gmail.com> 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.epochSecon...
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@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.epochSecon...
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.ht...
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
Hi Stephen, Peter, I think we should clarify the constructor to indicate that nanoseconds are truncated/ignored. That should be done as a separate request since it is a spec change and needs additional review. Roger On 4/29/2015 5:43 AM, Stephen Colebourne wrote:
On the LocalDateTime being passed in with nanoseconds, that was an unconsidered use case. The whole offset system relies on second based offsets, so it should really be validated/truncated to remove nanos. That way the equals/compareTo could be simplified again. Seems like a separate issue, but perhaps could be tackled here. You need an Oracle sponsor to tell you ;-)
Stephen
On 29 April 2015 at 10:33, Peter Levart <peter.levart@gmail.com> 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.epochSecon...
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@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.epochSecon...
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.ht...
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
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 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.epochSecon...
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@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.epochSecon...
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.ht...
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
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.epochSecon...
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@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.epochSecon...
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.ht...
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
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). 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.epochSecon...
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@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.epochSecon...
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.ht...
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
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.epochSecon... 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.epochSecon...
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@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.epochSecon...
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.ht...
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
The approach works for me, and the patch is valid as is. Stephen On 30 April 2015 at 11:24, Peter Levart <peter.levart@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.epochSecon...
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.epochSecon...
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@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.epochSecon... > > > 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.ht... > > > 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 >
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.epochSecon...
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.epochSecon...
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@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.epochSecon... > > > > 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.ht... > > > > 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 >
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.epochSecon...
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.epochSecon...
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@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.epochSecon... >> >> >> >> 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.ht... >> >> >> >> 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 >>
Hi Peter, Caching the epochSecond moves the computation from a relatively lazy version to creation when it would be performed eagerly for every transition. Is there a quick way to see if it will have an impact on the startup time? Roger On 4/27/2015 12:24 PM, Peter Levart 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.epochSecon...
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.ht...
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
TzdbZoneRulesProvider parses the byte[] of TZ data on demand when a ZoneId is first requested. So, the ZoneOffsetTransition will not be created unless a ZoneId is during startup. Stephen On 27 April 2015 at 22:58, Roger Riggs <Roger.Riggs@oracle.com> wrote:
Hi Peter,
Caching the epochSecond moves the computation from a relatively lazy version to creation when it would be performed eagerly for every transition. Is there a quick way to see if it will have an impact on the startup time?
Roger
On 4/27/2015 12:24 PM, Peter Levart 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.epochSecon...
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.ht...
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
participants (3)
-
Peter Levart
-
Roger Riggs
-
Stephen Colebourne