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

[Mikael Gerdin]

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

Sounds good.

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

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

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?

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