java.time.ZoneId.systemDefalut() overhead

[Xueming Shen]

On 02/23/2015 01:44 PM, Peter Levart wrote:
>
> On 02/23/2015 08:36 PM, Stephen Colebourne wrote:
>> Looking at the patch, all that may be needed is to change the type of
>> the instance variable used for the cache from ZoneId to Object, and
>> cast where necessary. Adds the cost of the cast to the main flow, but
>> that is pretty minimal.
>>
>> Stephen
>
> Having an instance variable in a class does not trigger loading of the class for the type of instance variable just yet.  Try the following with -verbose:class and you'll see that only Test.A is loaded...
>
> public class Test {
>
>     static class A {
>         B b;
>
>         B toB() {
>             if (b == null) {
>                 b = new B();
>             }
>             return b;
>         }
>     }
>
>     static class B {
>     }
>
>     public static void main(String[] args) throws Exception {
>         A a = new A();
>     }
> }
>
>
> If we're talking about using java.time from ZipEntry, then that's another story. I belive that VM startup does not need the conversion from DOS time to Java time in ZipEntry, but that should be checked to be sure...

I was talking about to use j.time for the dos<-> conversion, I would assume that
was the original optimization you were talking about. With Claes's change, it is
possible the j.u.zip code no longer needs to the dos->java time conversion during
startup.  The dos timestamp in Zip spec is indeed a j.time.LocalDateTime.


>
> Regards, Peter
>
>>
>>
>> On 23 February 2015 at 19:24, Xueming Shen <xueming.shen at oracle.com> wrote:
>>> I think I did have a j.time version somewhere when working on JSR310 and
>>> measured
>>> the difference of the performance . If my memory serves me correctly one of
>>> the issues
>>> back then is that it has an impact to the startup time, because using the
>>> JSR310 LDT
>>> and its timezone implementation means to load and initialize LOTS of the
>>> java.time
>>> classes and initialize the JSR310 tz data. We did lots of rounds of
>>> implementation of
>>> using the JSR310 tzdata for the java.util.TimeZone implementation in jdk8
>>> (so the
>>> JSR310 and j.u.TimeZone share the same tz data) and tried the best to use as
>>> less
>>> JSR310 classes as possible to limit the startup "regression".  It's true
>>> that it might not
>>> matter if the app is going to use the new JSR310 anyway, but for most of
>>> existing
>>> applications, you will see a slowdown of the startup without too much
>>> benefit. That
>>> was the consideration back to jdk8, in which the zip/jar was a must for
>>> anything to
>>> startup, if it's no longer true for jdk9 module system, then it might no
>>> longer be an
>>> concern.
>>>
>>> -Sherman
>>>
>>>
>>> On 02/22/2015 12:21 PM, 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
>>>>
>>>> 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
>>>>
>