java.time.ZoneId.systemDefalut() overhead

[Claes Redestad]

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
>