Hi Peter,

On 2015-02-22 21:21, Peter Levart 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

https://bugs.openjdk.java.net/browse/JDK-8066644 was filed to fix the deprecation warnings of practically identical code in jdk/nio/zipfs/ZipUtils.java, maybe this could be fixed at the same time?

I did a similar experiment recently and think I saw better throughput going from LocalDateTime to the sufficient second precision via toEpochSecond:

-    @SuppressWarnings("deprecation")
     public static long dosToJavaTime(long dtime) {
-        Date d = new Date((int)(((dtime >> 25) & 0x7f) + 80),
-                          (int)(((dtime >> 21) & 0x0f) - 1),
-                          (int)((dtime >> 16) & 0x1f),
-                          (int)((dtime >> 11) & 0x1f),
-                          (int)((dtime >> 5) & 0x3f),
-                          (int)((dtime << 1) & 0x3e));
-        return d.getTime();
+ LocalDateTime time = LocalDateTime.of((int) (((dtime >> 25) & 0x7f) + 1980),
+                (int) ((dtime >> 21) & 0x0f),
+                (int) ((dtime >> 16) & 0x1f),
+                (int) ((dtime >> 11) & 0x1f),
+                (int) ((dtime >> 5) & 0x3f),
+                (int) ((dtime << 1) & 0x3e));
+ return time.toEpochSecond(ZoneId.systemDefault().getRules().getOffset(time)) * 1000L;
     }

This avoids a few allocations.

Similarly for the javaToDos case going from an Instant to LocalDateTime with a ZoneOffset seemed to win out:

+        Instant instant = Instant.ofEpochMilli(time);
+        LocalDateTime d = LocalDateTime.ofInstant(instant,
+ ZoneOffset.systemDefault().getRules().getOffset(instant));


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?


Seems OK to me - perhaps an alternative would be to add a public static ZoneId getDefaultZoneId() on TimeZone rather than adding backdoors to the getDefaultRef? The caching seems sound, and I guess we don't have to worry about making it volatile.

The defensive copy in TimeZone.setDefault() seems like maybe it should be a separate bug.

Thanks!

/Claes


Regards, Peter


Reply via email to