Hi Claes,
On 02/22/2015 03:11 AM, Claes Redestad wrote:
Hi Sherman,
On 2015-02-21 19:49, Xueming Shen wrote:
Hi Claes,
This change basically undo the "fix" for 4759491 [1], for better
performance ...
my intent was never to break current behavior, but that mistake can be
rectified
without missing out on the startup benefit of laziness:
http://cr.openjdk.java.net/~redestad/jdk9/8073497/webrev.1/
In getLastModifiedTime():
207 public FileTime getLastModifiedTime() {
208 if (mtime != null)
209 return mtime;
210 if (time == -1)
211 return null;
212 return FileTime.from(dosToJavaTime(time),
TimeUnit.MILLISECONDS);
213 }
...you could also save the result of expression in line 212 to 'mtime'
so repeated calls would return cached instance of FileTime instead of
constructing new instance each time.
I would also rename 'time' field to 'dosTime' to aid in code readability.
Regards, Peter
The time/mtime getters now use the mtime field if it exists, while the
setters will
update both fields. Since getLastModified already fell back to
converting the
time field rather than null if mtime wasn't set, setting mtime to a
FileTime when
calling setTime seems consistent and a cheap way to preserve the time
precision.
I guess a range check to skip the FileTime creation in setTime if the
time is within
the DOS time bounds would be a valid optimization, since that will
typically
always be the case.
If we go with this change, I think we should also update the field
comment back to the
original one to clearly indicates the "time" is "in DOS time".
- long time = -1; // modification time (in DOS time)
+ long mtime = -1; // last modification time
Done.
The set/getLastModifiedTime() pair also need update to set/get the
"time" field
correctly to/from the dos time.
Done.
Thanks!
/Claes
-Sherman
[1] http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/90df6756406f
On 2/21/15 6:34 AM, Claes Redestad wrote:
Hi all,
please review this patch to re-introduce laziness in the java-to-dos
time
conversions for the ZipEntry.time field.
Webrev: http://cr.openjdk.java.net/~redestad/jdk9/8073497/webrev.0/
Bug: https://bugs.openjdk.java.net/browse/JDK-8073497
See bug for more details.
This behavior was actually the case before 8-b94, when the time field
was removed in favor of a set of FileTime fields, but when it was later
re-introduced to address some compatibility issues the conversion was
implemented in an eager fashion. This inadvertently affects VM startup
ever so little, since for every entry read via a ZipFile or
ZipInputStream
we'll do a relatively expensive call creating a Date and doing timezone
conversion.
Some gains from loading fewer classes during VM startup, as well.
Thanks!
/Claes
