On 01/09/2013 01:19 PM, Aleksey Shipilev wrote:
Good stuff.
On 01/07/2013 02:46 AM, David Holmes wrote:
http://cr.openjdk.java.net/~dholmes/8005232/webrev/
Sorry for obnoxious first-time reviewer questions:
a) I think the CAS loop in newReflectionData() is misleading in its
reuse of the parameters. Can we instead inline it? Or, can we read the
fields for $reflectionData and $classRedefinedCount, with no parameters
passed?
It was originally written as one method. Performance tests showed that
some (i think two more) of the public API methods were more aggressively
in-lined when split in two methods. I think it would not hurt
performance practically if the original variant was used.
b) Had we considered filling up the complete ReflectionData eagerly on
first access? This will make ReflectionData completely final. I would
guess this goes against the per-use laziness?
Certainly there are usages where only one part is ever needed. For
example only getDeclaredXXX but not getXXX which also triggers loading
in superclasses and then aggregating...
c) What's the merit of using Unsafe instead of field updaters here?
(Avoiding the dependency on j.u.c?)
Mainly because of initialization-loops. AtomicReferenceFieldUpdater uses
Class.getDeclaredField(name), which in turn needs ReflectionData, ...
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.
d) I think the code can be further simplified if we stick with
reflectionData() returning non-null object all the time, and check for
useCaches instead in the usages, at the expense of creating the methods
which actually get the reflection data.
I don't quite understand what you mean. Are you suggesting to double all
the methods with variants that don't use reflectionData? If you check
the usages you can see that the "caching aspect" is tested at two places
in each method, the rest of code is the same for caching/non-caching
variants.
e) Should useCaches be final? That will allow aggressive optimizations
for (c).
Again the initialization order issues (see checkInitted()). The value
for useCaches comes from system properties. Might be final and be later
overwritten via Unsafe, but wouldn't this defeat optimizations? (or if
not, wouldn't then those optimizations take the wrong code-path?)
Regards, Peter
This is a saving of 7 reference fields ie. 28 bytes, resulting in a new
Class instance size of 80 bytes. This saves a further 4 bytes due to the
fields being 8-byte aligned without any need for padding. So overall we
save 32 bytes per class instance.
Shameless promotion follows. This tool [1] should help to estimate the
class layout in the face of object alignment, compressed references,
paddings, alignment and whatnot.
-Aleksey.
[1] https://github.com/shipilev/java-object-layout/
