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

[Thomas Stüfe]

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

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