RFR(m): 8170933: Cleanup Metaspace Chunk manager: Unify treatment of humongous and non-humongous chunks

Thomas Stüfe thomas.stuefe at gmail.com
Thu Dec 15 07:12:35 UTC 2016


Dear all,

please take a look at this change.

Issue: https://bugs.openjdk.java.net/browse/JDK-8170933
webrev: http://cr.openjdk.java.net/~stuefe/webrevs/
8170933-unify-treatment-of-humongous-and-non-humongous-
chunks-on-chunkmanager-return/webrev.00/webrev/

This is a change destined for the upcoming jdk10, but I nevertheless hope
to get some reviews in already.

This is a follow up for a request by Mikael Gerdin when reviewing
JDK-8170520 "Make Metaspace ChunkManager counters non-atomic". He suggested
consolidating the chunk return process for humongous and non-humongous
chunks (see http://mail.openjdk.java.net/pipermail/hotspot-runtime-
dev/2016-November/021946.html)

Before, humongous chunks were returned in ~SpaceManager to the
ChunkManager-internal dictionary. Non-Humongous chunk returns were already
handled by the ChunkManager itself. That was code duplication and also
required exposing ChunkManager internals to the outside.

Now, the ChunkManager itself handles returning of both humongous and
non-humongous chunks. That allowed us to hide a lot of ChunkManager
internals from the outside: both freelists (for the non-humongous chunks)
and dictionary (for the humongous chunks) and their accessor methods are
now private.

Note that this change preceedes my proposed change for JDK-8170520 in order
to make coding for JDK-8170520 simpler. So, this change should apply to
jdk9/hs without needing any preceeding patches.

Implementation details:

There are now two functions: ChunkManager::return_single_chunk() and
ChunkManager::return_chunk_list(). I split them because we have a place
where we return single chunks (when retiring VirtualSpaceNode) and because
it felt cleaner to separate chain handling and returning chunks.

I would have actually preferred to get rid of the ChunkIndex parameter in
ChunkManager::return_single_chunk() and ::return_chunk_list() - just hand
down a Metachunk* or a chunk list and let the ChunkManager find out each
time the ChunkIndex for the returned chunk. That would have made the code
simpler in exchange for a little bit of performance loss. But because I
could not estimate the performance costs of determining the index for every
chunk instead of the chunk list, I refrained from this.

In order to make the ChunkManager-internal freelists private, I added a
method ChunkManager::size_by_index(), which returns chunk size for a given
index. This is the reverse of the already existing
ChunkManager::list_index(), which returns index by chunk size. Would have
liked to rename list_index() to something like index_by_size() but wanted
to keep the change small.

I attempted to preserve the logging where possible, and where I had to
change it, to make it better. So it will not look exactly the same as
before. In particular, it may be a bit more verbose.

I added a number of gtests to test the ChunkManager return process. I
followed the pattern of previous authors by adding the test to the end of
metaspace.cpp, but I am not happy about this solution. Seems that
metaspace.cpp consists of a lot of test coding by now. It should be
possible to move the test code outside to hotspot/test/native without
exposing all metaspace internals to the world - eg. by using simple
facades. This would also make it possible to use the gtest ASSERT_.. macros
instead of using hotspot assert(). However, I decided to leave this for a
separate cleanup.

Tests:

I built and successfully ran the gtests on a linux x64, Windows x86 and
x64, Solaris sparc and AIX. Also, our nightly test runs (jtreg among
others) indicated no problem which could be attributed to my change.

Thank you for reviewing,

Thomas


More information about the hotspot-runtime-dev mailing list