RFR(XS) 8210523 runtime/appcds/cacheObject/DifferentHeapSizes.java crash

Ioi Lam ioi.lam at oracle.com
Tue Sep 11 00:55:20 UTC 2018


Hi Jiangli,

Here's an updated webrev with the renaming:

http://cr.openjdk.java.net/~iklam/jdk12/8210523-DifferentHeapSizes-crash.v02/

Thanks

- Ioi


On 9/10/2018 5:29 PM, Jiangli Zhou wrote:
> On 9/10/18 5:20 PM, Ioi Lam wrote:
>
>>
>>
>> On 9/10/2018 5:08 PM, Jiangli Zhou wrote:
>>> Hi Ioi,
>>>
>>>
>>> On 9/10/18 4:23 PM, Ioi Lam wrote:
>>>>
>>>>
>>>> On 9/10/2018 10:37 AM, Jiangli Zhou wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> The fix looks correct. Please rename 
>>>>> start_address_with_archived_oop_encoding_mode() to 
>>>>> start_address_with_relocated_oop_encoding_mode(). The original 
>>>>> name might be a confusing factor that caused the bug (adding the 
>>>>> delta to the result of 
>>>>> start_address_with_archived_oop_encoding_mode()).
>>>>>
>>>>
>>>> start_address_with_archived_oop_encoding_mode() internally calls 
>>>> HeapShared::decode_with_archived_oop_encoding_mode(), so the naming 
>>>> is consistent there. I didn't want to use "relocated ..." because 
>>>> relocation may or may not happen. In many cases, delta == 0.
>>> With the default CDS archive, relocation occurs more often at 
>>> runtime unless smaller java heap size is explicitly specified. 
>>> start_address_with_archived_oop_encoding_mode() does causes 
>>> confusion. It took me a while to refresh my memory and realized it 
>>> not the 'true' dump time encoding. Maybe 
>>> start_address_with_adjusted_archived_oop_encoding_mode()?
>>
>> I think it's very hard to precisely describe what it does in a name 
>> that's shorter than a paragraph. So to avoid the misleading names:
>>
>>        HeapShared::decode_from_archive()
>>        FileMapInfo::start_address_as_decoded_from_archive()
>> FileMapInfo::start_address_as_decoded_with_current_oop_encoding_mode()
>>
>> If you want to know what "decode_from_archive()" does, you have to 
>> read the code.
>>
>> What do you think?
> decode_from_archive sounds ok.
>
> Thanks,
> Jiangli
>
>>
>> Thanks
>> - Ioi
>>
>>>>
>>>>
>>>>> Some more comments will be beneficial for readers who are not 
>>>>> familiar with the region relocation code. We should explain that 
>>>>> the first HeapShared::init_narrow_oop_decoding() call sets the 
>>>>> relocation decoding mode and all addresses decoded using it 
>>>>> afterwards are relocated. The second 
>>>>> HeapShared::init_narrow_oop_decoding() resets the relocation 
>>>>> decoding mode and adjusts alignment.
>>>>>
>>>>
>>>> Hmm, I could do
>>>>
>>>>   // sets the relocation decoding mode and all addresses decoded 
>>>> using the calculated delta
>>>>   log_info(cds)("CDS heap data relocation delta = " INTX_FORMAT " 
>>>> bytes", delta);
>>>>   HeapShared::init_narrow_oop_decoding(narrow_oop_base() + delta, 
>>>> narrow_oop_shift());
>>>>
>>>> But that doesn't seem to add any info that's not already in the 
>>>> code ....
>>>>
>>>> Commenting about this is tricky, and the involved code is rather 
>>>> small (just FileMapInfo::map_heap_regions_impl and 
>>>> HeapShared::decode_with_archived_oop_encoding_mode()). I feel that 
>>>> writing comments about what it does will just be translating the 
>>>> code into English, which will probably get out of date. I think it 
>>>> should be sufficient to understand the code with the existing 
>>>> comments and the -Xlog:cds output.
>>>>
>>>> What do you think?
>>> The code section is small, but the details are intricate. We can 
>>> improve the comments later also, I'll leave the decision to you.
>>>
>>> Thanks,
>>> Jiangli
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>> Thanks,
>>>>>
>>>>> Jiangi
>>>>>
>>>>> On 9/9/18 4:21 PM, Ioi Lam wrote:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8210523
>>>>>> http://cr.openjdk.java.net/~iklam/jdk12/8210523-DifferentHeapSizes-crash.v01/ 
>>>>>>
>>>>>>
>>>>>> The bug is here:
>>>>>>
>>>>>>    address relocated_strings_bottom = 
>>>>>> start_address_with_archived_oop_encoding_mode(si);
>>>>>> -  if (!is_aligned(relocated_strings_bottom + delta, 
>>>>>> HeapRegion::GrainBytes)) {
>>>>>> +  if (!is_aligned(relocated_strings_bottom, 
>>>>>> HeapRegion::GrainBytes)) {
>>>>>>
>>>>>> relocated_strings_bottom was already relocated, so you don't need 
>>>>>> the "+ delta"
>>>>>> here. The bug was hidden because in all of our test runs, delta 
>>>>>> had been
>>>>>> a multiple of HeapRegion::GrainBytes, so that didn't affect the 
>>>>>> result of the
>>>>>> is_align() call.
>>>>>>
>>>>>> Then, just by chance (probably due to Address Space Layout 
>>>>>> Randomization
>>>>>> on MacOS), we have a delta / GrainBytes == (29509025792 / 
>>>>>> 4194304.0) = 7035.5,
>>>>>> so the bug is uncovered.
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>



More information about the hotspot-runtime-dev mailing list