Hi,
On 12/06/2017 02:30 PM, Alan Bateman wrote:
I think this class is normally maintained on i18n-dev but I think
introducing the Cache object looks good and making this much easier to
understand.
-Alan
Thanks Alan, I'm forwarding to i18n-dev to see if maintainers of that
part of JDK have any comments...
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
