java.time.ZoneId.systemDefalut() overhead
Stephen Colebourne
scolebourne at joda.org
Thu Feb 26 22:47:53 UTC 2015
Personally, I found the SharedSecrets version easier to follow. I also
suspect that the toZoneId0() method would be invoked most of the time
with this variant, resulting in no beneficial inlining effect.
The ZoneOffsetTransition patch looks like a no-brainer to me.
Stephen
On 26 February 2015 at 20:14, Peter Levart <peter.levart at gmail.com> wrote:
> 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