Incorrect validation of DST in java.util.SimpleTimeZone

[Peter Levart]

Hi Venkat,

I created the following issue:

https://bugs.openjdk.java.net/browse/JDK-8191216

I can also sponsor this patch and push it for JDK 10.

The patch that you are proposing looks good to me. Does anybody have 
anything else to say?

Regards, Peter


On 11/13/2017 11:28 AM, Venkateswara R Chintala wrote:
> Thanks David, Peter for your review and comments. As I am new to the 
> community, can you please help me to open a bug and integrate the 
> changes into code base?
>
> -Venkat
>
> On 12/11/17 2:19 AM, Peter Levart wrote:
>> Hi David, Venkat,
>>
>> On 11/11/17 21:15, Peter Levart wrote:
>>> For example, take the following method:
>>>
>>> String defaultTZID() {
>>>     return TimeZone.getDefault().getID();
>>> }
>>>
>>> When JIT compiles it and inlines invocations to other methods within 
>>> it, it can prove that cloned TimeZone instance never escapes the 
>>> call to defaultTZID() and can therefore skip allocating the instance 
>>> on heap.
>>>
>>> But this is fragile. If code in invoked methods changes, they may 
>>> not get inlined or EA may not be able to prove that the cloned 
>>> instance can't escape and allocation may be introduced. 
>>> ZoneId.systemDefault() is a hot method and it would be nice if we 
>>> manage to keep it allocation free.
>>
>> Well, I tried the following variant of SimpleTimeZone.clone() patch:
>>
>>     public Object clone()
>>     {
>>         SimpleTimeZone tz = (SimpleTimeZone) super.clone();
>>         // like tz.invalidateCache() but without holding a lock on clone
>>         tz.cacheYear = tz.startYear - 1;
>>         tz.cacheStart = tz.cacheEnd = 0;
>>         return tz;
>>     }
>>
>>
>> ...and the JMH benchmark with gc profiling shows that 
>> ZoneId.systemDefault() still manages to get JIT-compiled without 
>> introducing allocation.
>>
>> Even the following (original Venkat's) patch:
>>
>>     public Object clone()
>>     {
>>         SimpleTimeZone tz = (SimpleTimeZone) super.clone();
>>         tz.invalidateCache();
>>         return tz;
>>     }
>>
>> ...does the same and the locking in invalidateCache() is elided too. 
>> Allocation and lock-elision go hand-in-hand. When object doesn't 
>> escape, allocation on heap may be eliminated and locks on that 
>> instance elided.
>>
>> So this is better than synchronizing on the original instance during 
>> .clone() execution as it has potential to avoid locking overhead.
>>
>> So Venkat, go ahead. My fear was unjustified.
>>
>> Regards, Peter
>>
>