RFR 8145628: hotspot metadata classes shouldn't use HeapWordSize or heap related macros like align_object_size

[Chris Plummer]

On 1/28/16 1:41 PM, Coleen Phillimore wrote:
>
> 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 
> debugger mostly.
>>
>> 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?
??? Any comment?
>>
>> 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 
>> Metachunk::object_alignment().
>
> 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.
Do you mean "just" because? I wasn't necessarily suggesting that all 
metadata be 64-bit aligned. However, the ones that have their allocation 
size 64-bit aligned should be. I think David's concern is that he wrote 
code that computes how much memory is needed for the archive, and it 
uses size() for that. If the Metachunk allocator allocates more than 
size() due to the 64-bit alignment of Metachunk::object_alignment(), 
then he will underestimate the size. You'll need to double check with 
David to see if I got this right.
> 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.
Correct.
>   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.
I agree with that, but just wanted to point out why David may be 
concerned with this change.
>>
>> instanceKlass.hpp: Need to fix the following comment:
>>
>>   97   // sizeof(OopMapBlock) in HeapWords.
> Fixed, Thanks!
thanks,

Chris
>
> Coleen
>
>>
>> thanks,
>>
>> Chris
>>
>>
>> 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
>>>
>>> thanks,
>>> Coleen
>>>
>>>
>>
>