Hi Aleksey,
Thanks for the feedback. Let me prefix this by saying that time is of
the essence here so unless there is a major issue things will go in as
is, with follow ups for after M6 if needed. We can't miss M6 but we can
tweak the initial changes after M6.
On 9/01/2013 10: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:
No problem, I prepared suitably dismissive responses ;-)
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?
As Peter commented this was out-lined for performance reasons. I can see
your point over the reuse of the parameters as local variables but I
don't have a problem with it. Arguably we save a couple of volatile
field reads on the fastpath.
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?
If we did that then every reflection using class would use more memory -
which defeats the whole point of this memory reduction exercise. ???
c) What's the merit of using Unsafe instead of field updaters here?
(Avoiding the dependency on j.u.c?)
Yes, as Peter said, initialization order bites you.
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'm not quite sure what you mean, but assuming these are just
stylistically different, there's no real motivation to make such a change.
e) Should useCaches be final? That will allow aggressive optimizations
for (c).
It can't be final as it is controlled via a property query that can't be
run during static initialization.
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.
I plan to take a look at this tool sometime - I promise. :)
Thanks,
David
-Aleksey.
[1] https://github.com/shipilev/java-object-layout/
