[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(m): 8170933: Cleanup Metaspace Chunk manager: Unify treatment of humongous and non-humongous
From:       Thomas_Stüfe <thomas.stuefe () gmail ! com>
Date:       2017-01-12 9:23:27
Message-ID: CAA-vtUxXB9rp_33XJ-BGkQzLbh43z3Ds9TuXUfM6BUaiTeyK9A () mail ! gmail ! com
[Download RAW message or body]

Could I have a review please? Or should I wait until jdk 10?

Thanks and Kind Regards, Thomas

On Thu, Dec 15, 2016 at 8:12 AM, Thomas St=C3=BCfe <thomas.stuefe@gmail.com=
>
wrote:

> 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 suggest=
ed
> 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 alread=
y
> 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 appl=
y
> 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 becaus=
e
> 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 eve=
ry
> 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 give=
n
> 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_.. macr=
os
> 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
>
>
>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]