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

[David Holmes]

Hi Coleen,

I think what Chris was referring to was the CDS compaction  work - which 
has since been abandoned. To be honest it has been so long since I was 
working on this that I can't recall the details. At one point Ioi 
commented how all MSO's were allocated with 8-byte alignment which was 
unnecessary, and that we could do better and account for it in the 
size() method. He also noted if we somehow messed up the alignment when 
doing this that it should be quickly detectable on sparc.

These current changes will affect the apparent wasted space in the 
archive as the expected usage would be based on size() while the actual 
usage would be determined by the allocator.

Ioi was really the best person to comment-on/review this.

David
-----

On 31/01/2016 12:27 AM, Coleen Phillimore wrote:
>
>
> On 1/29/16 2:20 PM, Coleen Phillimore wrote:
>>
>> Thanks Chris,
>>
>> On 1/29/16 2:15 PM, Chris Plummer wrote:
>>> Hi Coleen,
>>>
>>> On 1/28/16 7:31 PM, 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.
>>> ok.
>>>> 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.
>>> Ok. If you think there may be more, or a more thorough analysis is
>>> needed, perhaps just file a bug to get the rest later.
>>
>> From my look yesterday, there aren't a lot of HeapWordSize left. There
>> are probably still a lot of HeapWord* casts for things that aren't in
>> the Java heap.  This is a bigger cleanup that might not make sense to
>> do in one change, but maybe in incremental changes to related code.
>>
>>>
>>> As for reviewing your incremental changes, as long as it was just
>>> more changes of HeapWordSize to wordSize, I'm sure they are fine.
>>> (And yes, I did see that the removal of Symbol size alignment was
>>> also added).
>>
>> Good, thanks.
>>
>>>>
>>>> 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.
>>> Well, there a few issues being addressed by fixing align_object_size.
>>> Using align_object_size was incorrect from a code purity standpoint
>>> (it was used on values unrelated to java objects), and was also
>>> incorrect when ObjectAlignmentInBytes was not 8. This was the main
>>> motivation for making this change.
>>
>> Exactly. This was higher priority because it was wrong.
>>>
>>> The 3rd issue is that align_object_size by default was doing 8 byte
>>> alignment, and this wastes memory on 32-bit. However, as I mentioned
>>> there may be some dependencies on this 8 byte alignment due to the
>>> metaspace allocator doing 8 byte alignment. If you can get David to
>>> say he's ok with just 4-byte size alignment on 32-bit, then I'm ok
>>> with this change. Otherwise I think maybe you should stay with 8 byte
>>> alignment (including symbols), and file a bug to someday change it to
>>> word alignment, and have the metaspace allocator require that you
>>> pass in alignment requirements.
>>
>> Okay, I can see what David says but I wouldn't change Symbol back.
>> That's mostly unrelated to metadata storage and I can get 32 bit
>> packing for symbols on 32 bit platforms.  It probably saves more space
>> than the other more invasive ideas that we've had.
>
> This is reviewed now.  If David wants metadata sizing to change back to
> 64 bits on 32 bit platforms, it's a one line change.  I'm going to push
> it to get the rest in.
> Thanks,
> Coleen
>>
>> Thanks,
>> Coleen
>>
>>>>
>>>> Thanks for looking at this in detail.
>>> No problem. Thanks for cleaning this up.
>>>
>>> Chris
>>>>
>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>