java.time.ZoneId.systemDefalut() overhead
Peter Levart
peter.levart at gmail.com
Thu Feb 26 23:10:09 UTC 2015
On 02/26/2015 11:47 PM, Stephen Colebourne wrote:
> 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.
Not true. Once the toZoneId0() invokes baseZone.toZoneId() (on the base
instance which is attached to defaultTimeZone static field) the ZoneId
object will be set on both instances (base and clone). Next call to
TimeZone.getDefault() will create another clone of default TimeZone, but
this time, the cloned zoneId field will already point to cached ZoneId.
That and EA-based elimination of clone's allocation makes this variant
as performant as SharedSecrets based.
But I agree that it is not simple to follow all the possible code paths.
The benefit is that java.time is not dependent on ShareSecrets.
> The ZoneOffsetTransition patch looks like a no-brainer to me.
Good. I'll create two RFE issues in JIRA to track this.
Regards, Peter
> 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