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

[coleen.phillimore at oracle.com]

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::size_by_index() 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::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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>