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.lev...@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.lev...@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 > >
