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

[Chris Plummer]

On 1/30/16 6: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

Ok.

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