Hi,
Using routines for converting DOS time to Java time and back
(java.util.zip.ZipUtils.[dosToJavaTime,javaToDosTime]) as a base for
comparing deprecated java.util.Date API with new java.time API, here's a
JMH test with 3 distinct implementations of those routines using
java.util.Date, java.time.LocalDateTime and java.util.ZonedDateTime
respectively:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneId.systemDefault/ZipUtilsTest.java
Running the test in unpatched JDK9 reveals these results:
Benchmark Mode Samples Score Score
error Units
j.t.ZipUtilsTest.dosToJavaTime_Date avgt 15 95.644
4.241 ns/op
j.t.ZipUtilsTest.dosToJavaTime_LDT avgt 15 118.155
2.980 ns/op
j.t.ZipUtilsTest.dosToJavaTime_ZDT avgt 15 131.456
3.586 ns/op
j.t.ZipUtilsTest.javaToDosTime_Date avgt 15 74.692
1.709 ns/op
j.t.ZipUtilsTest.javaToDosTime_LDT avgt 15 134.116
4.396 ns/op
j.t.ZipUtilsTest.javaToDosTime_ZDT avgt 15 141.987
8.697 ns/op
Using LocalDateTime instead of ZonedDateTime is a little faster, but
does not make it as fast as using java.util.Date.
With a patch for caching of ZoneId on default TimeZone, which speeds up
repeated calls to ZoneId.systemDefault(), the results are as follows:
Benchmark Mode Samples Score Score
error Units
j.t.ZipUtilsTest.dosToJavaTime_Date avgt 15 95.590
2.354 ns/op
j.t.ZipUtilsTest.dosToJavaTime_LDT avgt 15 79.682
3.572 ns/op
j.t.ZipUtilsTest.dosToJavaTime_ZDT avgt 15 86.801
2.081 ns/op
j.t.ZipUtilsTest.javaToDosTime_Date avgt 15 75.096
1.178 ns/op
j.t.ZipUtilsTest.javaToDosTime_LDT avgt 15 91.919
2.191 ns/op
j.t.ZipUtilsTest.javaToDosTime_ZDT avgt 15 92.037
2.054 ns/op
dosToJavaTime routine using LocalDateTime outperforms java.util.Date
based. But javaToDosTime still lags behind. Profiling shows another
point that can be optimized. In ZoneRules.getOffset(Instant) 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 which makes the above test produce these results when
combined with ZoneId.systemDefault() optimization:
Benchmark Mode Samples Score Score
error Units
j.t.ZipUtilsTest.dosToJavaTime_Date avgt 15 92.291
2.903 ns/op
j.t.ZipUtilsTest.dosToJavaTime_LDT avgt 15 79.765
3.106 ns/op
j.t.ZipUtilsTest.dosToJavaTime_ZDT avgt 15 87.282
2.967 ns/op
j.t.ZipUtilsTest.javaToDosTime_Date avgt 15 76.235
2.651 ns/op
j.t.ZipUtilsTest.javaToDosTime_LDT avgt 15 73.115
2.567 ns/op
j.t.ZipUtilsTest.javaToDosTime_ZDT avgt 15 75.701
2.226 ns/op
For these tests I used another approach for speeding up
ZoneId.systemDefault() which doesn't use ShareSecrets. It changes only
TimeZone class so that base TimeZone instance is referenced from the
clone returned by TimeZone.getDefault(). Although TimeZone object is
unnecessarily cloned, it does not represent much overhead (probably the
allocation is optimized away by JIT as the clone never escapes from
ZoneId.systemDefault()):
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneId.systemDefault/webrev.02/
The patch to ZoneOffsetTransition class is straightforward:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransition.epochSecond/webrev.01/
So is this a worthy improvement? What do you think about the new
approach for optimizing ZoneId.systemDefault() compared to SharedSecrets
based:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneId.systemDefault/webrev.01/
(this one still has a NPE bug in timeZone.setDefault())
Regards, Peter
On 02/23/2015 12:50 PM, Stephen Colebourne wrote:
Having recently spent time on performance myself, I know that there
are a number of things in the java.time package that need some work.
Improving ZoneId.systemDefault() is definitely an improvement I
welcome. The change is along the lines of that I would have made,
involving a "secret" location to allow data to be exchanged (this code
needs to avoid the clone). The idea of adding a public method
TimeZone.getDefaultZoneId() is also one I'd be interested in if it
works.
On the patch, in TimeZone::toZoneId() I would use two methods:
public ZoneId toZoneId() {
ZoneId zId = zoneId;
if (zId == null) {
zoneId = zId = toZoneId0();
}
return zId;
}
private ZoneId toZoneId0() {
String id = getID();
if (ZoneInfoFile.useOldMapping() && id.length() == 3) {
if ("EST".equals(id)) {
zId = ZoneId.of("America/New_York");
} else if ("MST".equals(id)) {
zId = ZoneId.of("America/Denver");
} else if ("HST".equals(id)) {
zId = ZoneId.of("America/Honolulu");
} else {
zId = ZoneId.of(id, ZoneId.SHORT_IDS);
}
}
}
This should aid hotspot inlining (removing uncommon paths from main flow).
Does the code properly handle the static setDefault() method? I
suspect so, but will be worth a test.
If there's no undesirable side effect then caching zoneId
inside TimeZone looks reasonable to me - however I'm not an expert
of the field, and I got to learn that time/date can be more complex
than what I first thought ;-)
One thing I noticed is that there are two kinds of ZoneRulesProvider(s):
those that are static and allow their rules to be cached and the dynamic
ones, that can provide dynamic view on rules and therefore don't allow
caching of rules. But this aspect is encapsulated in ZoneRegion class (an
implementation of ZoneId). So I think an instance to ZoneRegion (or any
ZoneId) can be cached for indefinite time as long as it's id is the one that
we are looking for.
Yes, ZoneId can be safely cached as an immutable object (probably
never going to be a value type BTW). All current ZoneRegion instances
have static, not dynamic, rules.
As a final possibility, it might be possible to add a new subclass of
TimeZone that works directly off ZoneId. (sourcing the offset rules
from ZoneId instead of direct from the underlying data). However, the
mutable API of TimeZone probably makes it quite hard to get right.
On the specific ZIP code, using LocalDateTime rather than
ZonedDateTime may be faster as there are less objects to create.Claes'
code looks OK at first glance.
To get more performance, effort needs to be spent on LocalDate.of()
and LocalTime.of(). Both use very clean code, but it calls out to a
general routine that has to lookup boundary numbers. Because they are
general, they have quite deep call stacks, and inlining sometimes
fails to reach them due to inlining depth limits. For those two
critical pieces of code, the limits need to be inlined, something like
this:
public static LocalDate of(int year, int month, int dayOfMonth) {
if (year < Year.MIN_VALUE || year > Year.MAX_VALUE) {
throw new DateTimeException(genInvalidFieldMessage(YEAR, year))
}
if (month < 1 || month > 12) {
throw new DateTimeException(genInvalidFieldMessage(MONTH_OF_YEAR, month))
}
if (dayOfMonth < 1 || dayOfMonth > 31) {
throw new DateTimeException(genInvalidFieldMessage(DAY_OF_MONTH, dayOfMonth))
}
return create(year, month, dayOfMonth);
}
private String genInvalidFieldMessage(TemporalField field, long value) {
return "Invalid value for " + field + " (valid values " + this + "): " +
value;
}
I've not tested if this is faster, but I'd be surprised if it wasn't.
Stephen
On 22 February 2015 at 20:21, Peter Levart <peter.lev...@gmail.com> wrote:
Hi,
I noticed internal JDK class java.util.zip.ZipUtils uses deprecated
java.util.Date API for implementing two methods for converting DOS to Java
time and back. I thought I'd try translating them to use new java.time API.
Translation was straightforward, but I noticed that new implementations are
not on par with speed to old java.util.Date versions. Here's a JMH benchmark
implementing those two conversion methods with java.util.Date and
java.time.ZonedDateTime APIs:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneId.systemDefault/ZipUtilsTest.java
The results show the following:
Benchmark Mode Samples Score Score error
Units
j.t.ZipUtilsTest.dosToJavaTime_Date avgt 5 101.890 17.570
ns/op
j.t.ZipUtilsTest.dosToJavaTime_ZDT avgt 5 137.587 13.533
ns/op
j.t.ZipUtilsTest.javaToDosTime_Date avgt 5 67.114 10.382
ns/op
j.t.ZipUtilsTest.javaToDosTime_ZDT avgt 5 143.739 15.243
ns/op
Quick sampling with jvisualvm shows that a substantial time is spent
repeatedly obtaining ZoneId.systemDefault() instance. I checked the
implementation and came up with the following optimization:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneId.systemDefault/webrev.01/
TimeZone is a mutable object and has to be defensively cloned when
TimeZone.getDefault() is invoked. So there's no point in caching a ZoneId
instance inside TimeZone instance if we cache it on a clone that is thrown
away each time ZoneId.systemDefault() is invoked. I use SharedSecrets to
access the uncloned TimeZone.defaultTimeZone instance where caching of
ZoneId pays of.
I think that it was never meant to change TimeZone's ID (TimeZone.setID())
after such instance was put into operation (for example installed as default
time zone with TimeZone.setDefault()). Such use is unintended and buggy. So
I also changed the implementation of TimeZone.setDefault() to save a
defensive copy of TimeZone object as default time zone instead of a
reference to it.
With this patch, the DOS/Java time conversion benchmark shows an
improvement:
Benchmark Mode Samples Score Score error
Units
j.t.ZipUtilsTest.dosToJavaTime_Date avgt 5 97.986 18.379
ns/op
j.t.ZipUtilsTest.dosToJavaTime_ZDT avgt 5 85.010 10.863
ns/op
j.t.ZipUtilsTest.javaToDosTime_Date avgt 5 71.073 25.408
ns/op
j.t.ZipUtilsTest.javaToDosTime_ZDT avgt 5 95.298 17.620
ns/op
Is this patch correct or did I miss something from the internals of
java.time API that does not permit such caching?
Regards, Peter
