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

[Jiangli Zhou]

Looks good.

Thanks,

Jiangli


On 9/10/18 5:55 PM, Ioi Lam wrote:
> 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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>