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 22:50:08 UTC 2016
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.
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