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
