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

[Mikael Gerdin]

Hi Thomas,

This is the correct list but I suspect nobody has yet carved out the 
time to review this.
I'll look at it this week!

/Mikael

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/
>>>
>>> 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::return_chunks 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 humongous_dictionary() 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::return_chunks() 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::return_chunks" method into two new ones,
>>> "ChunkManager::return_single_chunk()" which returns a single chunk to
>>> the ChunkManager without walking the chunk chain, and
>>> "ChunkManger::return_chunk_list()" 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::return_chunk_list()"  for all
>>> its chunk lists.
>>>
>>> So now we can remove the public access to the humongous chunk dictionary
>>> in ChunkManger (ChunkManager::humongous_dictionary()) and make this
>>> function private.
>>>
>>> ----
>>>
>>> Then there was the fact that the non-humongous chunk lists were also
>>> exposed to public via ChunkManager::free_chunks(). 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::size_by_index(),
>>> which returns the size of the items of a chunk list given by its chunk
>>> index.
>>>
>>> So ChunkManager::free_chunks() could be made private too.
>>>
>>> ---
>>>
>>> The rest are minor changes:
>>>
>>> - made ChunkManager::find_free_chunks_list() private because it was not
>>> used from outside the class
>>> - Fixed an assert in dec_free_chunks_total()
>>> - 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/test_chunkManager.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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>