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
