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
