On 01/10/2013 02:25 AM, Mandy Chung wrote:
On 1/6/2013 2:46 PM, David Holmes wrote:
http://cr.openjdk.java.net/~dholmes/8005232/webrev/
This looks good to me. David - besides the footprint performance data,
do you observe any performance difference in your performance testing
(I think refworkload is what you ran)?
On 01/09/2013 01:19 PM, Aleksey Shipilev wrote:
On 01/09/2013 05:04 PM, Peter Levart wrote:
Strictly speaking, CAS is actually not needed here. Since we keep
classRedefinedCount snapshot inside the ReflectionData, any races that
would install "older" ReflectionData over "newer", would be quickly
caught at next invocation to reflectionData(). So if there are any
objections to usage of Unsafe, it can be removed and replaced by simple
volatile write.
Yes, I think that would be more clear without any adverse impacts on
performance. Also, that better expresses the intent of requiring the
visibility, not the atomicity of cache update.
I agree with Alekesey that it'd be simpler and clearer if CAS is replaced
with the simple volatile write. But this change will require another
round
of performance measurement to determine its impact. I'd suggest to
follow
up this cleanup in a separate bug and changeset.
Mandy
FWIW, I ran my micro benchmarks with the variant of using simple
volatile write instead of CAS and there was no visible differences. But
I don't have access to 256 core machines ;-). Anyway I think It should
not present any performance difference, since the rate of class
re-definitions is usually not very high...
Although the slow-path is a simple as:
this.reflectionData = new SoftReference<>(rd = new
ReflectionData<>(classRedefinitionCount));
return rd;
... I had to split this too into a separate method to retain the same
inlienability.
Such fine-tunnings tend to be lost in other sub-optimalities of the code
that implements public reflection API, such as copying the arrays and
array elements in bulk-returning methods and linear searches for the
elements in single-element returning methods...
There is another simplification of code possible. The method
checkInited() and its usages could be removed and it's logic
incorporated into the slow-path newReflectionData() method while adding
another boolean variable test into the fast-path.
Regards, Peter
