Incorrect validation of DST in java.util.SimpleTimeZone

[Peter Levart]

Hi David,

On 11/11/17 13:37, David Holmes wrote:
> Hi Peter,
>
> On 11/11/2017 8:06 PM, Peter Levart wrote:
>> Hi Venkateswara R Chintala,
>>
>> I would like to remind that TimeZone.clone() is also in the code path 
>> of java.time.ZoneId.systemDefault() where it was relied on to be 
>> optimized by JIT to never actually allocate the cloned TimeZone 
>> object by employing escape analysis.
>
> remind? Where is this documented?

Unfortunately it is not documented. My bad. I did test it at the time 
and determined that ZoneId.systemDefault(), with my patch applied, did 
not do allocations when JIT kicked-in. So we settled for an easier variant:

http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneId.systemDefault/webrev.04/

instead of a more complicated one that uses SharedSecrets to avoid cloning:

http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneId.systemDefault/webrev.01/

>
> And given:
>
>     public static TimeZone getDefault() {
>         return (TimeZone) getDefaultRef().clone();
>     }
>
> how can it not allocate??

When JIT compiles some method and inlines called methods, it also 
analyses all allocations performed to see if some of them can be proved 
to not escape the invocation of that compiled method. It then eliminates 
allocations of such objects on heap and rather generates code that keeps 
their state in registers or stack.

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.

Regards, Peter

>
> David
> -----
>
>> It would be nice to verify if this patch keeps that behavior. To test 
>> this, consider a JMH benchmark that simply invokes 
>> ZoneId.systemDefault() and returns it from the test method. 1st 
>> verify that unmodified code, when JITed, performs no allocations. 
>> Then test with modified code by applying the patch and see if there's 
>> any difference. Hint: use "-prof gc" command line options to run the 
>> benchmarks.jar.
>>
>> Regards, Peter
>>
>>
>> On 11/11/17 06:53, Venkateswara R Chintala wrote:
>>> Thanks Sean. I am pasting the patch here:
>>>
>>> --- old/src/java.base/share/classes/java/util/SimpleTimeZone.java 
>>> 2017-11-11 11:17:38.643867420 +0530
>>> +++ new/src/java.base/share/classes/java/util/SimpleTimeZone.java 
>>> 2017-11-11 11:17:38.375870421 +0530
>>> @@ -868,7 +868,11 @@
>>>       */
>>>      public Object clone()
>>>      {
>>> -        return super.clone();
>>> +        // Invalidate the time zone cache while cloning as it
>>> +        // can be inconsistent due to race condition.
>>> +        SimpleTimeZone tz = (SimpleTimeZone) super.clone();
>>> +        tz.invalidateCache();
>>> +        return tz;
>>>      }
>>>
>>>      /**
>>> --- /dev/null    2017-11-02 17:09:59.155627814 +0530
>>> +++ new/test/java/util/TimeZone/SimpleTimeZoneTest.java 2017-11-11 
>>> 11:17:38.867864912 +0530
>>> @@ -0,0 +1,55 @@
>>> +/*
>>> + * @test
>>> + * @summary Tests the race condition between 
>>> java.util.TimeZone.getDefault() and java.util.GregorianCalendar()
>>> + * @run main SimpleTimeZoneTest
>>> +*/
>>> +
>>> +import java.util.Calendar;
>>> +import java.util.GregorianCalendar;
>>> +import java.util.SimpleTimeZone;
>>> +import java.util.TimeZone;
>>> +
>>> +public class SimpleTimeZoneTest extends Thread {
>>> +    Calendar cal;
>>> +
>>> +    public SimpleTimeZoneTest (Calendar cal) {
>>> +        this.cal = cal;
>>> +    }
>>> +
>>> +    public static void main (String[] args) {
>>> +        TimeZone stz = new SimpleTimeZone(7200000, 
>>> "Asia/Jerusalem", Calendar.MARCH, 27, 0, 3600000, 
>>> Calendar.SEPTEMBER, 16, 0, 3600000);
>>> +        TimeZone.setDefault(stz);
>>> +
>>> +        SimpleTimeZoneTest stt = new SimpleTimeZoneTest(new 
>>> GregorianCalendar());
>>> +        stt.setDaemon(true);
>>> +        stt.start();
>>> +
>>> +        for (int i = 0; i < 50000; i++) {
>>> +            Calendar cal = new GregorianCalendar();
>>> +            cal.clear();
>>> +            cal.getTimeInMillis();
>>> +            cal.set(2014, 2, 2);
>>> +            cal.clear();
>>> +            cal.getTimeInMillis();
>>> +            cal.set(1970, 2, 2);
>>> +        }
>>> +
>>> +    }
>>> +
>>> +    public void run() {
>>> +        while (true) {
>>> +            cal.setTimeZone(TimeZone.getDefault());
>>> +            cal.clear();
>>> +            cal.set(2008, 9, 9);
>>> +            Calendar calInst = java.util.Calendar.getInstance();
>>> +            calInst.setTimeInMillis(cal.getTimeInMillis());
>>> +
>>> +            if (calInst.get(java.util.Calendar.HOUR_OF_DAY) != 
>>> cal.get(java.util.Calendar.HOUR_OF_DAY) ||
>>> +                calInst.get(java.util.Calendar.MINUTE) != 
>>> cal.get(java.util.Calendar.MINUTE) ||
>>> +                calInst.get(java.util.Calendar.SECOND) != 
>>> cal.get(java.util.Calendar.SECOND) ||
>>> +                calInst.get(java.util.Calendar.MILLISECOND) != 
>>> cal.get(java.util.Calendar.MILLISECOND)) {
>>> +                    throw new RuntimeException("Test failed");
>>> +            }
>>> +        }
>>> +    }
>>> +}
>>>
>>>
>>> On 10/11/17 9:29 PM, Seán Coffey wrote:
>>>> I think the OpenJDK mailing lists accept attachments if in patch 
>>>> format. If it's a simple short patch, it's acceptable to paste it 
>>>> into email body.
>>>>
>>>> Easiest solution is to use webrev[1]. If you can't upload this to 
>>>> cr.openjdk.java.net - then one of your colleagues may be able to help.
>>>>
>>>> [1] http://openjdk.java.net/guide/webrevHelp.html
>>>>
>>>> Regards,
>>>> Sean.
>>>>
>>>> On 10/11/17 12:18, Venkateswara R Chintala wrote:
>>>>> Looks like the patch attached earlier is not visible. As this is 
>>>>> my first contribution, please let me know how I can send the patch 
>>>>> for review.
>>>>>
>>>>>
>>>>> On 10/11/17 5:37 PM, Venkateswara R Chintala wrote:
>>>>>> Hi,
>>>>>>
>>>>>> In a multi-threaded environment, when java.util.SimpleTimeZone 
>>>>>> object is used to create a default timezone, there can be a race 
>>>>>> condition between the methods java.util.Timezone.getDefault() and 
>>>>>> java.util.Timezone.getDefaultRef() which can result in 
>>>>>> inconsistency of cache that is used to validate a particular 
>>>>>> time/date in DST.
>>>>>>
>>>>>> When a thread is cloning a default timezone object 
>>>>>> (SimpleTimeZone) and at the same time if a different thread 
>>>>>> modifies the time/year values, then the cache values (cacheYear, 
>>>>>> cacheStart, cacheEnd) can become inconsistent which leads to 
>>>>>> incorrect DST determination.
>>>>>>
>>>>>> We considered two approaches to fix the issue.
>>>>>>
>>>>>> 1)Synchronize access to cloning default timezone object when 
>>>>>> cache is being modified.
>>>>>>
>>>>>> 2)Invalidate the cache while returning the clone.
>>>>>>
>>>>>> We preferred the second option as synchronization is more expensive.
>>>>>>
>>>>>> We have attached the patch and jtreg testcase. Please review.
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>