RFR: 8170395: Metaspace initialization queries the wrong chunk freelist

[Thomas Stüfe]

On Mon, Nov 28, 2016 at 5:45 PM, Mikael Gerdin <mikael.gerdin at oracle.com>
wrote:

> Hi Stefan,
>
> 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/
>>
>
> Overall I think this change looks good.
> One thing I noticed is that the first parameter to
> VirtualSpaceList::get_new_chunk
> is actually ignored so you might want to just get rid of it, it's just
> confusing to see it. If you decide to do something about get_new_chunk I
> think it wouldn't hurt to have the names of the parameters changed as well,
> "grow_chunks_by_words" is actually "requested_chunk_size" and
> "medium_chunk_bunch" could be something like "suggested_commit_granularity"
>
>
+1 to that, this would make the code quite a bit clearer.

I also had a hard time understanding the "make_current" flag in
SpaceManager::add_chunk() until I (hope I) understood that it only matters
for humongous chunks where we differentiate between (a) preallocating a
still-unused humongous chunk for future allocations (initial chunk) or (b)
allocating a humongous chunk for immediate consumption by a
larger-than-medium-chunk memory request. I never saw (b) in real life,
however, the only humongous chunks I ever see are the initial chunks. Does
this ever happen?


> You might want to make the "const size_t" constants you moved out of the
> enum to either be "static" (which would be static in the C-sense) or add
> them in an anonymous namespace since otherwise they will pollute the global
> symbol namespace (more so than an enum which is strictly file scoped).
>
> The rest of the change looks good to me.
>
> /Mikael
>
>
> 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
>>
>