Hi,

On 2015-07-01 19:05, Xueming Shen wrote:
On 07/01/2015 05:02 AM, Claes Redestad wrote:
Hi,

On 2015-06-30 23:11, Xueming Shen wrote:
Hi,

Please help review and comment on this rfe.

Issue: https://bugs.openjdk.java.net/browse/JDK-8075526
webrev: http://cr.openjdk.java.net/~sherman/8075526

I think this looks reasonable.

I think we could consolidate the LocalDateTime<->xdostime
conversions into ZipUtils along with and aligned with the code
to convert UTC instants (ZipUtils::dosToJavaTime/javaToDosTime),
which I've suggested should be converted into similar code
anyhow:

I would prefer to deal with JDK-8066644 issue separately. One of the concerns of replacing the j.u.Date with the java.time classes was the possible startup time regression that may be triggered by forcing the vm to load lots of java.time classes given all the jvm starts with loading bunch of jar files. As a matter of fact, we spent the time and did the prototype when doing JSR310, it did show startup regression back then... This might no longer be an issue if moving to module system, but it'd be better to have some data first. On the other hand, the proposed change here is for two new methods, no impact to any existing
apps.


I fired up the patch proposed to address 8066644 on latest jdk9/dev (the work was paused while Peter Levart addressed some related inefficiencies, e.g., caching result in ZoneId.getDefault()) and have verified that replacing the current j.u.Date implementation with java.time in ZipUtils have no measurable effect on our internal startup and footprint benchmarks (with/without modules) since neither is no longer loaded on application startup.

I don't care if we move forward with 8066644 before or after this patch, justnoticedthat
the parts of the code from this and that patch looks very much the same.

Also spotted this nit:

+            this.mtime = FileTime.from(
+                ZonedDateTime.of(time, ZoneId.systemDefault()).toInstant());

could be written

+            this.mtime = FileTime.from(
+                time.atZone(ZoneId.systemDefault()).toInstant());


/Claes

Reply via email to