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

[Jiangli Zhou]

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