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

Coleen Phillimore coleen.phillimore at oracle.com
Mon Feb 1 20:59:47 UTC 2016 


On 2/1/16 2:59 PM, Chris Plummer wrote:
> 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.

Yes, exactly.  I think we need to add another parameter to 
Metaspace::allocate() to allow the caller to specify alignment requirements.

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

True.  Why I said "if wanted" above was that we'd need to file an RFE to 
reclaim the wasted memory.

Coleen

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