RFR JDK-8075526: Need a way to read and write time in UTC

[Claes Redestad]

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