Incorrect validation of DST in java.util.SimpleTimeZone

[Peter Levart]

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