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

David Holmes david.holmes at oracle.com
Wed Feb 3 11:10:07 UTC 2016 


On 2/02/2016 8:50 AM, Coleen Phillimore wrote:
>
>
> On 2/1/16 4:35 PM, Chris Plummer wrote:
>> On 2/1/16 1:24 PM, Coleen Phillimore wrote:
>>>
>>>
>>> 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.
>> Ah, I missed that Amalloc_4 does not do the size aligning. Interesting
>> that Amalloc_4 requires the size to be 4 bytes aligned, but then
>> Amalloc has no such requirement but will align to 2x the word size.
>> Lastly Amalloc_D aligns to 32-bit except on 32-bit sparc, where it
>> aligns to 64-bit. Sounds suspect. I thought 32-bit ARM VFP required
>> doubles to be 64-bit aligned.
>>> Not sure about 128 bit alignment for 64 bit systems, is that right?
>> #ifdef _LP64
>> const int LogBytesPerWord    = 3;
>> #else
>> const int LogBytesPerWord    = 2;
>> #endif
>>
>> So this means BytesPerWord is 8 on 64-bit, and 2*BytesPerWord is 16
>> (128-bit).
>
> Wow, this is excessive and unexpected.  We should file a bug.  I can't
> see a good reason to pad out arena allocations to 16 bytes.

Allocated blocks are (potentially) on distinct cache lines ?

David

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