RFR 8098821: Crash in system dictionary initialization with shared strings

[Jiangli Zhou]

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
>