Hi Peter,
On 6/12/2017 9:08 PM, Peter Levart wrote:
> Hi David,
>> Can I consider your comment as a Review? I'd like to get this patch into
> JDK10 if possible.
No sorry. I see what you're doing and I think it is okay but the regular
owners/maintainers of this code need to have the say on any changes here.
David
> Regards, Peter
>>> On 11/28/2017 08:17 AM, David Holmes wrote:
>> Hi Peter,
>>>> I like what you have done here. That said the general
>> thread-unsafeness of the code in SimpleTimeZone still causes me
>> concern - but what you are doing is not breaking anything more than it
>> is already broken.
>>>> David
>>>> On 25/11/2017 9:32 AM, Peter Levart wrote:
>>> Hi,
>>>>>> @Venkat: Sorry for late response, but I had to try something 1st.
>>>>>> This is an official request for reviewing a patch for fixing a data
>>> race between cloning a SimpleTimeZone object and lazily initializing
>>> its 3 cache fields which may produce a clone with inconsistent cache
>>> state. Here's Jira issue with details:
>>>>>>https://bugs.openjdk.java.net/browse/JDK-8191216>>>>>> Venkat has proposed to simply call invalidateCache() on the clone
>>> before returning it from SimpleTimeZone.clone() method:
>>>>>> public Object clone()
>>> {
>>> SimpleTimeZone clone = (SimpleTimeZone) super.clone();
>>> clone.invalidateCache();
>>> return clone;
>>> }
>>>>>> This fixes the issue and for the case of TimeZone.getDefault() which
>>> is called from ZoneId.systemDefault() even elides synchronization in
>>> clone.invalidateCache() and allocation of the clone, so JITed
>>> ZoneId.systemDefault() is unaffected by the patch. Initially I was
>>> satisfied with the fix, but then I tested something. Suppose someone
>>> sets default zone to SimpleTimeZone:
>>>>>> TimeZone.setDefault(
>>> new SimpleTimeZone(3600000,
>>> "Europe/Paris",
>>> Calendar.MARCH, -1, Calendar.SUNDAY,
>>> 3600000, SimpleTimeZone.UTC_TIME,
>>> Calendar.OCTOBER, -1, Calendar.SUNDAY,
>>> 3600000, SimpleTimeZone.UTC_TIME,
>>> 3600000)
>>> );
>>>>>> And then calls some methods that initialize the cache of the internal
>>> shared TimeZone.defaultTimeZone instance:
>>>>>> new Date().toString();
>>>>>> The code which after that tries to obtain default time zone and
>>> calculate the offset from UTC at some current point in time, for
>>> example:
>>>>>> TimeZone.getDefault().getOffset(now)
>>>>>> can't use the cached state because it has been invalidated in the
>>> returned clone. Such code has to re-compute the offset every time it
>>> gets new clone. I measured this with a JMH benchmark and got the
>>> following result:
>>>>>> Default:
>>>>>> Benchmark Mode Cnt Score Error Units
>>> ZoneIdBench.TimeZone_getDefault_getOffset avgt 10 57,168 ± 0,501
>>> ns/op
>>> ZoneIdBench.ZoneId_systemDefault avgt 10 3,558 ± 0,040 ns/op
>>>>>> Venkat's patch - invalidateCache():
>>>>>> Benchmark Mode Cnt Score Error Units
>>> ZoneIdBench.TimeZone_getDefault_getOffset avgt 10 179,476 ± 1,942
>>> ns/op
>>> ZoneIdBench.ZoneId_systemDefault avgt 10 3,538 ± 0,073 ns/op
>>>>>> We see that ZoneId.systemDefault() is unaffected, but
>>> TimeZone.getDefault().getOffset(now) becomes 3x slower.
>>>>>> This is not good, so I tried an alternative fix for the issue -
>>> simply making the SimpleTimeZone.clone() synchronized. Access to
>>> cache fields is already synchronized, so this should fix the race.
>>> Here's the JMH result:
>>>>>> Default:
>>>>>> Benchmark Mode Cnt Score Error Units
>>> ZoneIdBench.TimeZone_getDefault_getOffset avgt 10 57,168 ± 0,501
>>> ns/op
>>> ZoneIdBench.ZoneId_systemDefault avgt 10 3,558 ± 0,040 ns/op
>>>>>> Synchronized clone():
>>>>>> Benchmark Mode Cnt Score Error Units
>>> ZoneIdBench.TimeZone_getDefault_getOffset avgt 10 58,103 ± 0,936
>>> ns/op
>>> ZoneIdBench.ZoneId_systemDefault avgt 10 4,154 ± 0,034 ns/op
>>>>>> We see that caching works again, but synchronization has some
>>> overhead which is not big for single-threaded access, but might get
>>> bigger when multiple threads try to get default zone concurrently.
>>>>>> So I created a 3rd variant of the fix which I'm presenting here and
>>> requesting a review for:
>>>>>>http://cr.openjdk.java.net/~plevart/jdk10-dev/8191216_SimpleTimeZone_clone_race/webrev.01/>>>>>>>>> The JMH benchmark shows this:
>>>>>> Default:
>>>>>> Benchmark Mode Cnt Score Error Units
>>> ZoneIdBench.TimeZone_getDefault_getOffset avgt 10 57,168 ± 0,501
>>> ns/op
>>> ZoneIdBench.ZoneId_systemDefault avgt 10 3,558 ± 0,040 ns/op
>>>>>> Cache object:
>>>>>> Benchmark Mode Cnt Score Error Units
>>> ZoneIdBench.TimeZone_getDefault_getOffset avgt 10 42,860 ± 0,274
>>> ns/op
>>> ZoneIdBench.ZoneId_systemDefault avgt 10 3,545 ± 0,057 ns/op
>>>>>> Not only does the fix not affect ZoneId.systemDefault() which is not
>>> surprising, but it also speeds-up cache lookup in single-threaded
>>> benchmark and certainly eliminates possible contention among threads
>>> looking up the shared instance.
>>>>>> I have run the test/jdk/java/util/TimeZone and
>>> test/jdk/java/util/Calendar jtreg tests and there were 7 failures
>>> caused by compilation errors (package sun.util.locale.provider is not
>>> visible), while 59 other tests pass.
>>>>>> So, what do you think?
>>>>>> Regards, Peter
>>>>>>>>> Venkateswara R Chintala je 21. 11. 2017 ob 10:14 napisal:
>>>> 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
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>