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