Incorrect validation of DST in java.util.SimpleTimeZone

[Venkateswara R Chintala]

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
>