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

[Chris Plummer]

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.

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

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