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

[Ioi Lam]

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?

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