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

[Ioi Lam]

Hi Jiangli, Thanks for the review!

- Ioi


On 9/10/18 6:14 PM, Jiangli Zhou wrote:
> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>