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:35:52 UTC 2016
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).
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