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

Mikael Gerdin mikael.gerdin at oracle.com
Mon Mar 13 16:59:32 UTC 2017

Hi Thomas,

I've had a quick look at the patch, overall it looks good but I haven't looked at the tests yet.

On 2017-03-11 10:08, Thomas Stüfe wrote:

Ping... Did I send this to the wrong mailing list?

On Tue, Mar 7, 2017 at 3:01 PM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:

(Resent to hotspot-dev)

On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:

Hi all,

I would like to get a review for this cleanup change for the metaspace.cpp. This has been a long time brewing in my backlog, but I had to wait until jdk10 is open. The cleanup is actually quite small, the largest part of the change is the added test coding. Issue: https://bugs.openjdk.java.net/browse/JDK-8170933 Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE -1-8170933-unify-treatment-of-humongous-and-non-humongous-ch unks-on-chunkmanager-return/jdk10-webrev.00/webrev/

While you're at it, would you mind breaking

assert(_free_chunks_count > 0 && _free_chunks_total >= v, "About to go negative");

into two separate asserts and have them print the values?

In ChunkManager::list_index you can now use size_by_index (and maybe make size_by_index inline in the ChunkManager class declaration?

In ChunkManager::return_chunk_list you might as well use LogTarget(Trace, gc, metaspace, freelist) log; since you only log to the trace level anyway.

I'll get back to you with feedback on the tests once I have had a good look at them.

Thanks for picking up this fix! /Mikael

The change implements a suggestion made by Mikael Gerdin when reviewing JDK-8170520 last year, who suggested that the ChunkManager class should handle returns of both non-humongous and humongous chunks, see original discussion here: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2 016-November/021949.html "It would be great if you could have a look at unifying the ChunkManager::returnchunks API for standard and humongous chunks so that we wouldn't need the special casing for this in ~SpaceManager That way we could also make humongousdictionary() private to ChunkManager which would improve encapsulation." The cleanup does this and a bit more, all changes having to do with the fact that the ChunkManager class unnecessarily exposes internals to the other classes in metaspace.cpp and that with a little bit of reorganization this can be avoided. The benefit is simpler coding and way better encapsulation, which will help a lot with future changes (in preparation for https://bugs.openjdk.java.net/browse/JDK-8166690). -------- The changes in detail: The main issue was that ChunkManager::returnchunks() did not handle humongous chunks. To return humongous chunks, one had to access the humongous chunk dictionary inside the ChunkManager and add the chunk yourself, including maintaining all counters. This happened in ~SpaceManager and made this destructor very complicated. This is solved by moving the handling of humongous chunk returns to the ChunkManager class itself. For convenience, I split the old "ChunkManager::returnchunks" method into two new ones, "ChunkManager::returnsinglechunk()" which returns a single chunk to the ChunkManager without walking the chunk chain, and "ChunkManger::returnchunklist()" which returns a chain of chunks to the ChunkManager. Both functions are now able to handle both non-humongous and humongous chunks. ~SpaceManager() was reimplemented (actually, quite simplified) and just calls "ChunkManager::returnchunklist()" for all its chunk lists. So now we can remove the public access to the humongous chunk dictionary in ChunkManger (ChunkManager::humongousdictionary()) and make this function private. ---- Then there was the fact that the non-humongous chunk lists were also exposed to public via ChunkManager::freechunks(). The only reason was that when a VirtualSpaceNode is retired, it accessed the ChunkManager free lists to find out the size of the items of the free lists. This was solved by adding a new function, ChunkManager::sizebyindex(), which returns the size of the items of a chunk list given by its chunk index. So ChunkManager::freechunks() could be made private too. --- The rest are minor changes: - made ChunkManager::findfreechunkslist() private because it was not used from outside the class - Fixed an assert in decfreechunkstotal() - I added logging (UL, with the tags "gc, metaspace, freelist"). I tried to keep the existing logging intact or add useful logging where possible. ---- A large chunk of the changes is the added gtests. There is a new class now, ChunkManagerReturnTest, which stresses the ChunkManager take/return code. Note that I dislike the fact that this test is implemented inside metaspace.cpp itself. For now I kept my new tests like the existing tests inside this file, but I think these tests should be moved to test/native/memory/testchunkManager.cpp. I suggest doing this in a separate fix though, because it needs a bit of remodeling (the tests need to access internals in metaspace.cpp). ---- I ran gtests and the jtreg hotspot tests on Linux x64, Solaris sparc and AIX. No issues popped up which were associated with my change. Thanks for reviewing and Kind Regards, Thomas

More information about the hotspot-dev mailing list