RFR 8145628: hotspot metadata classes shouldn't use HeapWordSize or heap related macros like align_object_size
coleen.phillimore at oracle.com
Thu Jan 28 21:41:11 UTC 2016
Thank you, Chris for looking at this change.
On 1/28/16 4:24 PM, Chris Plummer wrote:
> Hi Coleen,
> Can you do some testing with ObjectAlignmentInBytes set to something
> other than 8?
Okay, I can run one of the testsets with that. I verified it in the
> Someone from GC team should apply your patch, grep for
> align_object_size(), and confirm that the ones you didn't change are
> correct. I gave a quick look and they look right to me, but I wasn't
> always certain if object alignment was appropriate in all cases.
thanks - this is why I'd changed the align_object_size to
align_heap_object_size before testing and changed it back, to verify
that I didn't miss any.
> I see some remaining HeapWordSize references that are suspect, like in
> Array.java and bytecodeTracer.cpp. I didn't go through all of them
> since there are about 428. Do they need closer inspection?
> align_metadata_offset() is not used. It can be removed.
Okay, I'll remove it. That's a good idea.
> Shouldn't align_metadata_size() align to 64-bit like
> align_object_size() did, and not align to word size? Isn't that what
> we agreed to? Have you tested CDS? David had concerns about the
> InstanceKlass::size() not returning the same aligned size as
I ran the CDS tests but I could test some more with CDS. We don't
want to force the size of objects to be 64 bit (especially Symbol)
because Metachunk::object_alignment() is 64 bits. Unfortunately, with
the latter, metadata is never aligned on 32 bit boundaries for 32 bit
platforms, but to fix this, we have to pass a minimum_alignment
parameter to Metaspace::allocate() because the alignment is not a
function of the size of the object but what is required from its
nonstatic data members. I found MethodCounters, Klass (and subclasses)
and ConstantPool has such alignment constraints. Not sizing metadata
to 64 bit sizes is a start for making this change.
> instanceKlass.hpp: Need to fix the following comment:
> 97 // sizeof(OopMapBlock) in HeapWords.
> On 1/27/16 10:27 AM, Coleen Phillimore wrote:
>> Summary: Use align_metadata_size, align_metadata_offset and
>> is_metadata_aligned for metadata rather
>> than align_object_size, etc. Use wordSize rather than HeapWordSize
>> for metadata. Use align_ptr_up
>> rather than align_pointer_up (all the related functions are ptr).
>> Ran RBT quick tests on all platforms along with Chris's Plummers
>> change for 8143608, ran jtreg hotspot tests and nsk.sajdi.testlist
>> co-located tests because there are SA changes. Reran subset of this
>> after merging.
>> I have a script to update copyrights on commit. It's not a big
>> change, just mostly boring. See the bug comments for more details
>> about the change.
>> open webrev at http://cr.openjdk.java.net/~coleenp/8145628.01/
>> bug link https://bugs.openjdk.java.net/browse/JDK-8145628
More information about the hotspot-dev