java.time.ZoneId.systemDefalut() overhead
Peter Levart
peter.levart at gmail.com
Thu Feb 26 20:14:49 UTC 2015
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.levart at 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
>>
More information about the core-libs-dev
mailing list