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

Tue Mar 14 15:20:13 UTC 2017

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-8170933-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-8170933-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::size_by_index() 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 (size_by_index(), list_index(), and the two new
ones, ChunkManager::return_single_chunk ChunkManager::return_chunk_list).

@Mikael:

>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?

Done. Note that the counters (.. _count) being of size_t kind of irritates
me :) but I refrained from changing the type because touching that would
spread all over. Maybe in a separate cleanup.

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

Done.

>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.

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.

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::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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>