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

[Peter Levart]

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
>>
>