java.time.ZoneId.systemDefalut() overhead

[Stephen Colebourne]

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
>