RFR: 8170395: Metaspace initialization queries the wrong chunk freelist

Stefan Karlsson stefan.karlsson at oracle.com
Tue Nov 29 11:47:26 UTC 2016


Hi Per,

On 2016-11-29 12:11, Per Liden wrote:
> Hi Stefan,
>
> On 2016-11-28 22:06, Stefan Karlsson wrote:
>> Hi again,
>>
>> This set of patches resolve some of the comments given by Mikael and
>> Thomas:
>>
>> Entire patch:
>>  http://cr.openjdk.java.net/~stefank/8170395/webrev.02
>
> Looks good, just a few comments.
>
> metaspace.cpp
> -------------
>
>  751   static size_t specialized_chunk_size(bool is_class) { return
> (size_t) is_class ? ClassSpecializedChunk : SpecializedChunk; }
>  752   static size_t small_chunk_size(bool is_class)       { return
> (size_t) is_class ? ClassSmallChunk : SmallChunk; }
>  753   static size_t medium_chunk_size(bool is_class)      { return
> (size_t) is_class ? ClassMediumChunk : MediumChunk; }
>
> The size_t casts above binds to is_class and not the result from ?: so
> you probably you want to do:
>
> return is_class ? (size_t)A : (size_t)B;
>
> ... or perhaps just skip the casts.
>

Sure. This cast existed before my changes, but I can remove it since 
it's obviously wrong.

>
>  760   size_t specialized_chunk_size() { return
> specialized_chunk_size(is_class()); }
>  761   size_t small_chunk_size()       { return
> small_chunk_size(is_class()); }
>  762   size_t medium_chunk_size()      { return
> medium_chunk_size(is_class()); }
>  763
>  764   size_t smallest_chunk_size()    { return
> smallest_chunk_size(is_class()); }
>  765
>  766   size_t medium_chunk_bunch()     { return medium_chunk_size() *
> MediumChunkMultiple; }
>
> More of a style thing, but it looks like these functions could also be
> const, no?

Yes, and many other functions in that file. I'll update these since I 
changed them.

>
> I don't need to see a new webrev.

Thanks for reviewing,
StefanK

>
> cheers,
> Per
>
>>
>> Delta patches:
>>  http://cr.openjdk.java.net/~stefank/8170395/webrev.02.01.verify_global_initialization
>>
>>
>>  http://cr.openjdk.java.net/~stefank/8170395/webrev.02.02.revert_enum
>>  http://cr.openjdk.java.net/~stefank/8170395/webrev.02.03.unused_parameter
>>
>>
>> I consider pushing the last patch as a separate changeset.
>>
>> This is the entire patch without the unused_parameter patch:
>>  http://cr.openjdk.java.net/~stefank/8170395/webrev.02.01-02
>>
>> Thanks,
>> StefanK
>>
>> On 2016-11-28 14:52, Stefan Karlsson wrote:
>>> Hi all,
>>>
>>> Please, review this patch to fix metaspace initialization.
>>>
>>> http://cr.openjdk.java.net/~stefank/8170395/webrev.01/
>>> https://bugs.openjdk.java.net/browse/JDK-8170395
>>>
>>> The fix for JDK-8169931 introduced a new assert to ensure that we
>>> always try to allocate chunks that are any of the three fixed sizes
>>> (specialized, small, medium) or a humongous chunk (if it is larger
>>> then the medium chunk size).
>>>
>>> During metaspace initialization an initial metaspace chunk is
>>> allocated. The size of some of the metaspace instances can be
>>> specified on the command line. For example:
>>> java -XX:InitialBootClassLoaderMetaspaceSize=30720 -version
>>>
>>> If this size is smaller than the medium chunk size and at the same
>>> time doesn't match the specialized or small chunk size, then we end up
>>> hitting the assert mentioned above:
>>> #
>>> # Internal Error
>>> (/scratch/opt/jprt/T/P1/142848.erik/s/hotspot/src/share/vm/memory/metaspace.cpp:2359),
>>>
>>> pid=31643, tid=31646
>>> # assert(size > free_chunks(MediumIndex)->size()) failed: Not a
>>> humongous chunk
>>> #
>>>
>>> ========================================================================
>>>
>>> The most important part of the fix is this line:
>>> +  // Adjust to one of the fixed chunk sizes (unless humongous)
>>> +  const size_t adjusted = adjust_initial_chunk_size(requested);
>>>
>>> which ensures that we always request either of a specialized, small,
>>> medium, or humongous chunk size, even if the requested size is neither
>>> of these.
>>>
>>> Most of the other code is refactoring to unify the non-class metaspace
>>> and the class metaspace code paths to get rid of some of the existing
>>> code duplication, bring the chunk size calculation nearer to the the
>>> actual chunk allocation, and make it easier to write a unit test for
>>> the new adjust_initial_chunk_size function.
>>>
>>> ========================================================================
>>>
>>> The patch for JDK-8169931 was backed out with JDK-8170355 and will be
>>> reintroduced as JDK-8170358 when this patch has been reviewed and
>>> pushed.
>>>
>>> Testing: jprt, unit test, parts of PIT testing (including CDS tests),
>>> failing test
>>>
>>> Thanks,
>>> StefanK
>>
>>


More information about the hotspot-dev mailing list