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

Thomas Stüfe thomas.stuefe at gmail.com
Wed Mar 15 13:34:01 UTC 2017

Hi Mikael, Coleen,

On Wed, Mar 15, 2017 at 1:34 PM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:

Hi Thomas,

On 2017-03-14 16:20, Thomas Stüfe wrote: Hi Coleen, Mikael,

thanks again for looking over this change, I know its a lot of stuff. Coleen, thanks for offering sponsorship! Here the new webrev after your first input: webrev 01: http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-1-81709 33-unify-treatment-of-humongous-and-non-humongous-chunks-on- chunkmanager-return/jdk10-webrev.01/webrev/ delta 00-01: http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-1-81709 33-unify-treatment-of-humongous-and-non-humongous-chunks-on- chunkmanager-return/jdk10-webrev.00-to-01/webrev/ I addressed your issues: @Coleen can you put ChunkManager::sizebyindex() function definition up in the

file with the other ChunkManager functions? I moved this function and three stray others up to the rest of the ChunkManager functions (sizebyindex(), listindex(), and the two new ones, ChunkManager::returnsinglechunk ChunkManager::returnchunklist). @Mikael: While you're at it, would you mind breaking

assert(freechunkscount > 0 && freechunkstotal >= v, "About to go negative"); into two separate asserts and have them print the values?

Done. Note that the counters (.. count) being of sizet kind of irritates me :) but I refrained from changing the type because touching that would spread all over. Maybe in a separate cleanup. Sounds good. In ChunkManager::listindex you can now use sizebyindex (and maybe make sizebyindex inline in the ChunkManager class declaration? Done. In ChunkManager::returnchunklist you might as well use LogTarget(Trace, gc, metaspace, freelist) log; since you only log to the trace level anyway. Ok, did that. Note that there are a couple of more places where this could be done, but I did not touch those to keep the change small and reviewable. Ok. I've had a look at the test now and while I think it's valuable to have I'd like us to have some sort of actual unit test also. The test you added is more of a stress test as you state in the comment.

How would they differ from the tests I did? I am all in favor of more tests, it makes sense to have them now before larger changes to metaspace.cpp happens. I am just curious about how you would test, and what, exactly.

Perhaps now is also the time to rip out the classes local to metaspace.cpp in order to make them actually testable without having to do this complex dance with calling extern functions from the test code.

Yes! This can be easily isolated as an own separate issue. To track it, I opened: https://bugs.openjdk.java.net/browse/JDK-8176808

I'm not going to force you to do the move in this change but I'd really like us to do that in 10 now that we have the chance to do some cleanups, what do you think Thomas? Coleen?

I really would like to wrap this change here up. I prefer to do both moving the test code out of metaspace.cpp (JDK-8176808) and adding the unit tests you requested as separate follow up issues.

I still have three more changes to metaspace on the back burner: A) the delayed https://bugs.openjdk.java.net/browse/JDK-8170520 ("Make ChunkManager counters non-atomic") - this one is a very small change, especially after the cleanup done in this change. I will post this once this change got comitted. B) two more changes, one to add something called a "metaspace map", a way to print an ascii representation of the metaspace. And as follow up, the prototype for the new allocator. The "metaspace map" will be a way to easily see the effect of the new allocator (inspect the metaspace fragmentation).

I would propose to add the unit tests you wanted in between (A) and (B).

Kind Regards, Thomas

/Mikael Kind Regards, Thomas

On Mon, Mar 13, 2017 at 8:54 PM, <coleen.phillimore at oracle.com> wrote:

Hi Thomas, Thank you for cleaning up the metaspace code. I think the change looks good. I have only one comment and that is: can you put ChunkManager::sizebyindex() function definition up in the file with the other ChunkManager functions? If Mikael reviews the test, I'd be happy to sponsor this for you. Sorry about the inattention. thanks! Coleen

On 3/11/17 4:08 AM, 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/ 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