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

[Coleen Phillimore]

Thank you, Dmitry!  I checked it in before seeing this so couldn't list 
you as a reviewer.
Coleen

On 2/1/16 2:37 AM, Dmitry Samersoff wrote:
> Coleen,
>
> Didn't look over all changes but SA changes looks good to me.
>
> -Dmitry
>
> On 2016-01-29 06:31, Coleen Phillimore wrote:
>> 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
>>>>>>
>>>>>>
>