On 2015-07-06 19:07, Xueming Shen wrote:
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.
Sure. If backporting is on the table I think it would be more convenient
to get 8066644 in first
(since this change will block on getting ccc approval etc). I will send
out an RFR soon, unless
anyone objects or Peter beats me to it. :-)
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.
I was really only considering readability here. This will be path not
taken in most cases -
if not all - so outlining like I suggested could actually win by poking
the JIT to do the
right thing and ignore that branch. :-)
Thanks!
/Claes
-sherman
