RFR 8098821: Crash in system dictionary initialization with shared strings

Coleen Phillimore coleen.phillimore at oracle.com
Mon Jun 22 20:07:36 UTC 2015


Thank you, Jiangli.
Coleen


On 6/19/15 6:01 AM, Jiangli Zhou wrote:
> Hi Coleen,
>
> Thank you so much for fixing this! The changes look correct to me.
>
> Sent from my iPad
>
>> On Jun 17, 2015, at 1:43 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>>
>> Thank you Dan for the quick review.
>>
>>> On 6/17/15 4:27 PM, Daniel D. Daugherty wrote:
>>>> On 6/17/15 1:24 PM, Coleen Phillimore wrote:
>>>> Summary: map string regions after the compressed class base is known
>>>>
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8098821
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8098821_jdk.01/
>>> test/lib/sun/hotspot/WhiteBox.java
>>>     No comments.
>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8098821_hotspot.01/
>>> src/share/vm/memory/filemap.hpp
>>>     No comments.
>>>
>>>
>>> src/share/vm/memory/filemap.cpp
>>>     L666:                      "size %dM.", max_heap_size() >> 20);
>>>         Nit: There's got to be a macro to convert from size-in-bytes
>>>         to size-in-MB...
>> Most code does number/M which makes sense but this >>20 I guess is faster.  I'll change it to /M because who cares how fast it is?
>>
>>> L686:         StringTable::ignore_shared_strings(true);
>>>         Shouldn't this function have 'set_' as a prefix?
>>>
>>>         Update: So the "getter" is "StringTable::shared_string_ignored()"
>>>         and the "setter" is "StringTable::ignore_shared_strings(true)".
>>>         That seems inconsistent with HotSpot style, but the naming
>>>         pre-dates this change so OK.
>> Yes, I didn't write this....
> Those were introduced by me. I'll rename them to be more consistent with hotspot style.
>
> Thanks!
>
> Jiangli
>
>>> L724:       // the shared string data is mapped successfuly
>>>         nit typo: 'successfuly' -> 'successfully'
>> Fixed.
>>> src/share/vm/memory/metaspace.cpp
>>>     L3300:     if (UseSharedSpaces) {
>>> <snip>
>>>     L3308:       if (mapinfo->initialize() && MetaspaceShared::map_shared_spaces(mapinfo)) {
>>> <snip>
>>>     L3323:       } else {
>>>     L3324:         assert(!mapinfo->is_open() && !UseSharedSpaces,
>>>     L3325:                "archive file not closed or shared spaces not disabled.");
>>>
>>>         The assert() on L3324 is in the else-block for the
>>>         if-statement on L3308. That's inside the if-block
>>>         on L3300 so I don't see how the "&& !UseSharedSpaces"
>>>         part of the assert() is working here.
>> If map_shared_spaces() fails it will turn UseSharedSpaces off.  It's a nifty side effect.
>>
>>>         For what it's worth, it looks like the original code
>>>         had a similar problem.
>>>
>>> src/share/vm/memory/metaspaceShared.cpp
>>>     old L1004:       mapinfo->map_string_regions() &&
>>>         So I'm guessing that this map_string_regions() call has
>>>         migrated to Metaspace::global_initialize() in metaspace.cpp.
>> yes, it moved from MetaspaceShared::map_shared_spaces().
>>> old L1005:       mapinfo->verify_string_regions() &&
>>>         I'm not seeing where this verify_string_regions() call migrated.
>>>         Is it no longer needed?
>> That came also from metaspaceShared.cpp map_shared_spaces.
>>> old L1017:     mapinfo->unmap_string_regions();
>>>         I'm not see where this unmap_string_regions() call migrated.
>>>         Is it no longer needed?
>> Oh, one thing I didn't mention is that I'm going to file another RFE to follow the mapping failure cases, and make sure they are tested properly.   This function is called from
>>
>> void FileMapInfo::stop_sharing_and_unmap(const char* msg)
>>
>> which is called from metaspace.cpp
>>   if (UseSharedSpaces && !can_use_cds_with_metaspace_addr(metaspace_rs.base(), cds_base)) {
>>
>> I'm not sure these paths are well tested, but I wanted to make sure this crash goes away.
>>
>> Thank you for being so fast to code review this!
>>
>> Coleen
>>
>>> src/share/vm/prims/whitebox.cpp
>>>     No comments.
>>>
>>>
>>> Dan
>>>
>>>
>>>> Tested with failing tests on solaris and linux and JPRT.  The failing scenario and code impact is limited, so running more tests wouldn't exercise this code.
>>>>
>>>> New test code and white box API contributed by Misha.
>>>>
>>>> Thanks,
>>>> Coleen



More information about the hotspot-runtime-dev mailing list