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