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

[Coleen Phillimore]

Hi Chris,

I made a few extra changes because of your question that I didn't answer 
below, a few HeapWordSize became wordSize.  I apologize that I don't 
know how to create incremental webrevs.  See discussion below.

open webrev at http://cr.openjdk.java.net/~coleenp/8145628.02/

On 1/28/16 4:52 PM, Chris Plummer wrote:
> 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?

Actually, I tried to get a lot of HeapWordSize in the metadata but the 
primary focus of the change, despite the title, was to fix 
align_object_size wasn't used on metadata.  That said a quick look at 
the instances of HeapWordSize led to some that weren't in the heap.  I 
didn't look in Array.java because it's in the SA which isn't 
maintainable anyway, but I changed a few.  There were very few that were 
not referring to objects in the Java heap. bytecodeTracer was one and 
there were a couple in metaspace.cpp.

The bad news is that's more code to review.   See above webrev link.

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

I don't know what code this is but yes, it would be wrong.  It also 
would be wrong if there's any other alignment gaps or space in metaspace 
chunks because chunks themselves have an allocation granularity.

It could be changed back by changing the function align_metaspace_size 
from 1 to WordsPerLong if you wanted to.

I fixed Symbol so that it didn't call align_metaspace_size if this 
change is needed in the future.

I was trying to limit the size of this change to correct 
align_object_size for metadata.

Thanks for looking at this in detail.

Coleen


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