RFR: 8170395: Metaspace initialization queries the wrong chunk freelist

[Stefan Karlsson]

Hi Thomas,

On 2016-11-28 17:48, Thomas Stüfe wrote:
> Hi Stefan,
>
> This looks good.

Thanks.

> Some small remarks:
>
> - Metaspace::verify_initialized () : could be made static. For clarity 
> I'd also either rename it to something like 
> "verify_global_initialization" or to just roll the code out into its 
> only caller, Metaspace::initialize().

I'll rename the function and make it static.

Personally, I want verbose verification and debugging code to get out of 
the way of the other code. That's why I moved it to a separate function.

>
> - I never liked the ChunkSizes enum names, because they do not 
> indicate they are sizes, and now that the encompassing enum name 
> "ChunkSizes" is gone they are even less clear. Would it be possible to 
> rename the former enum values to "...Size" for better code clarity, 
> e.g. "MediumChunkSize" instead of "MediumChunk"?

I sort of agree, but changing it will affect large parts of 
metaspace.cpp, which makes it hard to see the other changes in this 
patch. I'd rather revert back to the enum, and maybe deal with that 
cleanup as a separate enhancement.

>
> - Metaspace::get_space_manager(MetadataType mdtype) - asserting for 
> mdType==Class||NonClassType instead of != MetadaTypeCount could be a 
> bit clearer.

The assert is copied from the other getters in the file, so I'd like to 
keep it for consistency.

Maybe we should get rid of MetadataTypeCount and that assert, and let 
the code that converts back and forth between MetadataType and integers 
do the assert check? That would need to be handled as a separate 
enhancement.

>
> - SpaceManager::adjust_initial_chunk_size () - could we rename this to 
> a more generic name like "::next_larger_chunksize" or similar?

I choose the name because it is a helper for a specific use-case and 
call site. I also considered giving it a more generic name, but I 
couldn't immediately come up with a name that accurately described the 
function. The proposed next_larger_chunksize isn't describing the 
function correctly, since adjust_initial_chunk_size(SmallChunk) returns 
SmallChunk and not MediumChunk. If we can figure out a spot-on name, I'd 
be happy to change it.
> I also wonder whether this could be combined somehow with 
> SpaceManager::calc_chunk_size(), which wants to do something similar 
> (calculate a fitting chunk size for a given smaller allocation size)

I briefly thought about that as well, but then skipped that though 
because of the heuristics involved in calc_chunk_size().

Thanks reviewing,
StefanK

>
>
> Kind Regards, Thomas
>
>
> On Mon, Nov 28, 2016 at 2:52 PM, Stefan Karlsson 
> <stefan.karlsson at oracle.com <mailto:stefan.karlsson at oracle.com>> wrote:
>
>     Hi all,
>
>     Please, review this patch to fix metaspace initialization.
>
>     http://cr.openjdk.java.net/~stefank/8170395/webrev.01/
>     <http://cr.openjdk.java.net/%7Estefank/8170395/webrev.01/>
>     https://bugs.openjdk.java.net/browse/JDK-8170395
>     <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
>
>