On 07/06/2015 07:15 AM, Claes Redestad wrote:
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.
Thanks for taking time on this. But let's do it separately. Just in case
8066644 might need
to be backport.
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());
@Override
public ZonedDateTime atZone(ZoneId zone) {
return ZonedDateTime.of(this, zone);
}
Just tried to save one layer of invocation :-) though probably does not matter.
The
chained/short code may look more decent.
-sherman
