RFR: 8170395: Metaspace initialization queries the wrong chunk freelist

[Per Liden]

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.


  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?

I don't need to see a new webrev.

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