(jdk10) RFR(m): 8170933: Cleanup Metaspace Chunk manager: Unify treatment of humongous and non-humongous chunks
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Wed Mar 15 14:56:11 UTC 2017
On 3/15/17 9:34 AM, Thomas Stüfe wrote:
> Hi Mikael, Coleen,
>
> On Wed, Mar 15, 2017 at 1:34 PM, Mikael Gerdin
> <mikael.gerdin at oracle.com <mailto:mikael.gerdin at oracle.com>> wrote:
>
> Hi Thomas,
>
>
> On 2017-03-14 16:20, Thomas Stüfe wrote:
>
> Hi Coleen, Mikael,
>
> thanks again for looking over this change, I know its a lot of
> stuff.
> Coleen, thanks for offering sponsorship!
>
> Here the new webrev after your first input:
>
> webrev 01:
> http://cr.openjdk.java.net/~stuefe/webrevs/METASPACE-1-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, <coleen.phillimore at oracle.com
> <mailto:coleen.phillimore at oracle.com>> wrote:
>
>
> Hi Thomas, Thank you for cleaning up the metaspace code.
> I think the
> change looks good. I have only one comment and that is:
> can you put
> ChunkManager::size_by_index() function definition up in
> the file with the
> other ChunkManager functions?
>
> If Mikael reviews the test, I'd be happy to sponsor this
> for you. Sorry
> about the inattention.
>
> thanks!
> Coleen
>
>
> On 3/11/17 4:08 AM, Thomas Stüfe wrote:
>
> Ping... Did I send this to the wrong mailing list?
>
> On Tue, Mar 7, 2017 at 3:01 PM, Thomas Stüfe
> <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>>
> wrote:
>
> (Resent to hotspot-dev)
>
>
> On Tue, Mar 7, 2017 at 7:45 AM, Thomas Stüfe
> <thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com>>
> wrote:
>
> Hi all,
>
>
> I would like to get a review for this cleanup
> change for the
> metaspace.cpp. This has been a long time
> brewing in my backlog, but I
> had
> to wait until jdk10 is open. The cleanup is
> actually quite small, the
> largest part of the change is the added test
> coding.
>
> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8170933
> <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
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
More information about the hotspot-dev
mailing list