Incorrect validation of DST in java.util.SimpleTimeZone

[Venkateswara R Chintala]

Thanks Peter for sponsoring this patch. Is there anything else that 
needs to be done from my end for this patch to be integrated? Please let 
me know.

-Venkat


On 14/11/17 8:46 PM, Peter Levart wrote:
> 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
>>>
>>
>
>

-- 
Regards
-Venkat