RFR 8098821: Crash in system dictionary initialization with shared strings

Coleen Phillimore coleen.phillimore at oracle.com
Wed Jun 17 20:43:36 UTC 2015


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

> 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