RFR(S): 8073497: Lazy conversion of ZipEntry time
Xueming Shen
xueming.shen at oracle.com
Sun Feb 22 21:16:52 UTC 2015
On 2/21/15 6:11 PM, 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/
>
> 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.
It's a reasonable solution. I would assume we probably need that "range
check" optimization
to avoid setting the "mtime" field in normal use scenario. ZOS now
outputs the more accurate
"mtime" to the zip file using "extend timestamp" if it's not null (the
entry gets bigger). The
assumption now is that we only output the extended timestamp if the time
stamps set
via the setXYZTime() explicitly.
ZOS.java
...
432 int elenEXTT = 0; // info-zip extended timestamp
433 int flagEXTT = 0;
434 if (e.mtime != null) {
435 elenEXTT += 4;
436 flagEXTT |= EXTT_FLAG_LMT;
437 }
438 if (e.atime != null) {
439 elenEXTT += 4;
440 flagEXTT |= EXTT_FLAG_LAT;
441 }
442 if (e.ctime != null) {
443 elenEXTT += 4;
444 flagEXTT |= EXTT_FLAT_CT;
445 }
446 if (flagEXTT != 0)
447 elen += (elenEXTT + 5); // headid(2) + size(2) + flag(1) + data
448 writeShort(elen);
...
-Sherman
>> 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
>>
>
More information about the core-libs-dev
mailing list