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

[Dmitry Samersoff]

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.