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 21:20:06 UTC 2016 

On 2/1/16 12:59 PM, Coleen Phillimore wrote:
>
>
> 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.
And I was looking at  Amalloc() also, which does:

     x = ARENA_ALIGN(x);

And then the following defines:

#define ARENA_ALIGN_M1 (((size_t)(ARENA_AMALLOC_ALIGNMENT)) - 1)
#define ARENA_ALIGN_MASK (~((size_t)ARENA_ALIGN_M1))
#define ARENA_ALIGN(x) ((((size_t)(x)) + ARENA_ALIGN_M1) & ARENA_ALIGN_MASK)

#define ARENA_AMALLOC_ALIGNMENT (2*BytesPerWord)

I think this all adds up to Amalloc doing 64-bit size alignment on 
32-bit systems and 128-bit alignment on 64-bit systems. So I'm not so 
sure you Symbol changes are having an impact, at least not for Symbols 
allocated out of Arenas. If I'm reading the code right, symbols created 
for the null ClassLoader are allocated out of an arena and all others 
out of the C heap.

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