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

Mon Feb 1 18:01:50 UTC 2016

On 2/1/16 12:56 PM, Chris Plummer wrote:
> It seems the allocators always align the size up to at least a 64-bit 
> boundary, so doesn't that make it pointless to attempt to save memory 
> by keeping the allocation request size word aligned instead of 64-bit 
> aligned?

Sort of, except you need a size as a multiple of 32 bit words to 
potentially fix this, so it's a step towards that (if wanted).

Coleen

>
> Chris
>
> On 1/31/16 4:18 PM, David Holmes wrote:
>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>