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

Wed Mar 15 15:32:14 UTC 2017

On Wed, Mar 15, 2017 at 3:56 PM, <coleen.phillimore at oracle.com> wrote:

>
>
> On 3/15/17 9:34 AM, Thomas Stüfe wrote:
>
> 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_lis
>>> t).
>>>
>>> @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?
>
>
> YES, please!
>
>
> 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 think it would be a good follow up fix to break up the tests.  The
> reason I didn't review it very well was because it's too big.
>
> I'm happy to sponsor this change as-is.
>
> Thank you Thomas for working on metaspace.cpp.  Compared to some of the
> other code in hotspot, it's a new piece of code and still needs shaking out
> and cleaning up.
>
> Coleen
>
>
Thank you both for the review work! We are getting there.

I sometimes wonder whether it would be simpler to plug in a completely new
allocator, but I suspect after a while we would arrive at the same complex
coding, because the usage scenario is complex and this would be reflected
in the allocator coding. So, we would not have gained much.

Kind Regards, Thomas

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