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

Chris Plummer chris.plummer at oracle.com
Mon Feb 1 19:59:55 UTC 2016 

On 2/1/16 11:54 AM, Chris Plummer wrote:
> On 2/1/16 10:01 AM, Coleen Phillimore wrote:
>>
>>
>> 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).
> What you need is (1) don't automatically pad the size up to 64-bit 
> alignment in the allocator, (2) don't pad the size up to 64-bit in the 
> size computations, and (3) the ability for the allocator to maintain 
> an unaligned "top" pointer, and to fix the alignment if necessary 
> during the allocation. This last one implies knowing the alignment 
> requirements of the caller, so that means either passing in the 
> alignment requirement or having allocators configured to the alignment 
> requirements of its users. You need all 3 of these. Leave any one out 
> and you don't recoup any of the wasted memory. We were doing all 3. 
> You eliminated at least some of the cases of (2).
Sorry, my wording near then end there was kind of backwards. I meant we 
were NOT doing any of the 3. You made is so in some cases we are now 
doing (2).

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

More information about the hotspot-dev mailing list