Quantcast

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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
(Resent to hotspot-dev)

On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]>
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-
> chunks-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
>
>
>
>
>
>
>
>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Ping... Did I send this to the wrong mailing list?

On Tue, Mar 7, 2017 at 3:01 PM, Thomas Stüfe <[hidden email]>
wrote:

> (Resent to hotspot-dev)
>
> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]>
> 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
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mikael Gerdin
Hi Thomas,

This is the correct list but I suspect nobody has yet carved out the
time to review this.
I'll look at it this week!

/Mikael

On 2017-03-11 10:08, 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 <[hidden email]>
> wrote:
>
>> (Resent to hotspot-dev)
>>
>> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]>
>> 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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Thank you Mikael!

On Mon, Mar 13, 2017 at 9:25 AM, Mikael Gerdin <[hidden email]>
wrote:

> Hi Thomas,
>
> This is the correct list but I suspect nobody has yet carved out the time
> to review this.
> I'll look at it this week!
>
> /Mikael
>
>
> On 2017-03-11 10:08, 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 <[hidden email]>
>> wrote:
>>
>> (Resent to hotspot-dev)
>>>
>>> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]>
>>> 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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mikael Gerdin
In reply to this post by Thomas Stüfe-2
Hi Thomas,

I've had a quick look at the patch, overall it looks good but I haven't
looked at the tests yet.

On 2017-03-11 10:08, 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 <[hidden email]>
> wrote:
>
>> (Resent to hotspot-dev)
>>
>> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]>
>> 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/

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?

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

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.

I'll get back to you with feedback on the tests once I have had a good
look at them.

Thanks for picking up this fix!
/Mikael

>>>
>>> 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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi Mikael,

thanks a lot for reviewing! The changes you request make sense, but I'll
wait until you reviewed the rest.

Kind Regards, Thomas

On Mon, Mar 13, 2017 at 5:59 PM, Mikael Gerdin <[hidden email]>
wrote:

> Hi Thomas,
>
> I've had a quick look at the patch, overall it looks good but I haven't
> looked at the tests yet.
>
> On 2017-03-11 10:08, 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 <[hidden email]>
>> wrote:
>>
>> (Resent to hotspot-dev)
>>>
>>> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]>
>>> 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/
>>>>
>>>
> 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?
>
> In ChunkManager::list_index you can now use size_by_index (and maybe make
> size_by_index inline in the ChunkManager class declaration?
>
> 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.
>
> I'll get back to you with feedback on the tests once I have had a good
> look at them.
>
> Thanks for picking up this fix!
> /Mikael
>
>
>
>>>> 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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by Thomas Stüfe-2

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 <[hidden email]>
> wrote:
>
>> (Resent to hotspot-dev)
>>
>> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]>
>> 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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi Coleen,

thank you for reviewing! I'll do the proposed change.

Kind Regards, Thomas

On Mon, Mar 13, 2017 at 8:54 PM, <[hidden email]> 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 <[hidden email]>
>> wrote:
>>
>> (Resent to hotspot-dev)
>>>
>>> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]>
>>> 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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
In reply to this post by coleen.phillimore
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, <[hidden email]> 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 <[hidden email]>
>> wrote:
>>
>> (Resent to hotspot-dev)
>>>
>>> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]>
>>> 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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: (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, <[hidden email]> 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 <[hidden email]>
>>> wrote:
>>>
>>> (Resent to hotspot-dev)
>>>>
>>>> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]>
>>>> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi Mikael, Coleen,

On Wed, Mar 15, 2017 at 1:34 PM, Mikael Gerdin <[hidden email]>
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, <[hidden email]> 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 <[hidden email]>
>>>> wrote:
>>>>
>>>> (Resent to hotspot-dev)
>>>>
>>>>>
>>>>> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]>
>>>>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore


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
> <[hidden email] <mailto:[hidden email]>> 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-8170933-unify-treatment-of-humongous-and-non-humongous-chunks-on-chunkmanager-return/jdk10-webrev.01/webrev/
>         <http://cr.openjdk.java.net/%7Estuefe/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/
>         <http://cr.openjdk.java.net/%7Estuefe/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.
>
>
> 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

>
> 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, <[hidden email]
>         <mailto:[hidden email]>> 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
>                 <[hidden email] <mailto:[hidden email]>>
>                 wrote:
>
>                 (Resent to hotspot-dev)
>
>
>                     On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe
>                     <[hidden email]
>                     <mailto:[hidden email]>>
>                     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
>                         <https://bugs.openjdk.java.net/browse/JDK-8170933>
>                         Webrev:
>                         http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE
>                         <http://cr.openjdk.java.net/%7Estuefe/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
>                         <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
>                         <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
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
On Wed, Mar 15, 2017 at 3:56 PM, <[hidden email]> 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 <[hidden email]>
> 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, <[hidden email]> 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 <[hidden email]>
>>>>> wrote:
>>>>>
>>>>> (Resent to hotspot-dev)
>>>>>
>>>>>>
>>>>>> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe <[hidden email]
>>>>>> >
>>>>>> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>
>
>
Loading...