RFR(S): 8073497: Lazy conversion of ZipEntry time

[Claes Redestad]

On 2015-02-23 16:41, Claes Redestad wrote:
> On 02/22/2015 10:16 PM, Xueming Shen wrote:
>> 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);
>> ...
>
> Here's my attempt to resolve this: 

We could preserve precision by shifting the remaining milliseconds into 
the dostime field,
since the DOS time will only use the 4 lower bytes:

http://cr.openjdk.java.net/~redestad/jdk9/8073497/webrev.4/

After I went through more extensive testing and verification I realized 
that narrowing
dostime to an int and breaking this remainder hack out into a separate 
field would be
possible without affecting ZipEntry footprint; arguably cleaner, but if 
this version is
acceptable I would prefer not to.

Going this way preserves precision across the entire input range of 
setTime, thus all tests
I could find pass with this version; I've also expanded the 
TestExtraTime test to test both
far ancient, far future and near future times to verify precision is 
preserved.

I backpedaled on the getLastModifiedTime optimization to set the mtime 
field that Peter
suggested since it would have the side-effect that the extra time field 
would be written
after a call to this getter, which would be a bit too unexpected.

Thanks!

/Claes