Final RFR: 8005232 (JEP-149) Class Instance size reduction

[Peter Levart]

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/
>