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 21:24:08 UTC 2016 


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

The Symbol arena version of operator 'new' calls Amalloc_4.  Not sure 
about 128 bit alignment for 64 bit systems, is that right?

Coleen

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