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

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

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

Thomas Stüfe-2
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/
2016-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
(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
>
>
>
>
>
>
>
>
>
>
>
>
Loading...