RFR(L): 8176808: Split up metaspace.cpp

[coleen.phillimore at oracle.com]

Thomas, thank you for doing this and sorry about the delay in looking at 
this.

On 5/9/18 11:08 AM, Thomas Stüfe wrote:
> Hi all,
>
> This was a long time coming. Lets cut up this beast :)
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8176808
> Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8176808-split-metaspace-cpp/webrev.00/webrev/
>
> This change splits up metaspace.cpp into multiple parts. Note that I
> did not change much code (exceptions see below in details) - so this
> is mostly moving code around.
>
> This change follows the scheme tentatively outlined in JDK-8201572:
>
> - metaspace internal classes go into the sub directory
> share/memory/metaspace. Ideally, those should never be accessed from
> outside, apart from other metaspace classes and metaspace tests. They
> also go into the namespace ::metaspace::internals.
>
> - metaspace external classes (MetaspaceUtils, ClassLoaderMetaspace,
> etc) remain inside share/memory and, for now, remain in the global
> namespace.
>
> Note: the original idea was to move metaspace external classes to
> namespace ::metaspace. But I shied away from that, since that would
> mean fixing up all usages of these classes either with metaspace::
> scope or with using declarations. I had to make a cut at some point.
>
> So here is something to decide:
> - should we then get rid of the ::metaspace::internals namespace, move
> metaspace-internal classes to the enclosing ::metaspace namespace and
> leave external classes in the global namespace ?
> - or should we follow through (maybe in a future patch): internal
> classes in ::metaspace::internal, and move external classes to
> ::metaspace ?

Yes I think "internal" is kind of noisy.  I like just the metaspace 
namespace to hold all these things, and keep the rest global.
>
> Some more details:
>
> - the following classes moved out of metaspace.cpp into namespace
> "metaspace::internal" and the metaspace sub directory:
> MetaDebug, ChunkManager, SpaceManager, BlockFreeList and SmallBlocks,
> OccupancyMap, VirtualSpaceNode and VirtualSpaceList, the
> PrintCLDMetaspaceInfoClosure.
>
> - I also moved metachunk.hpp to internals, since class Metachunk is
> not used externally. What is used externally is Metablock though, so I
> split up metachunk.hpp into metabase.hpp, metablock.hpp and
> metachunk.hpp, holding Metabase, Metablock and Metachunk,
> respectively.
>
> - Now we see some ugly seams: metaspace.hpp is supposed to be the
> outside API contract, however, it contains implementation details
> which should not be there and which manifest now by having to use the
> metaspace::internals namespace here and there. If we care, we could
> solve this with a bit redesigning.
>
> - The ChunkManager gtest and the SpaceManager gtest had been
> implemented inside metaspace.cpp, since it was not possible to access
> the internal classes those tests needed in any other way. Since that
> is now possible, I moved the coding out to gtest-land and made them
> real gtests (exchanging the asserts for real gtest ASSERTS, among
> other things).
> Note that there are some tests left in metaspace.cpp
> (TestMetaspaceUtilsTest, TestVirtualSpaceNodeTest) - those were no
> gtests-in-disguise but were part of -XX:+ExecuteInternalVMTests. I
> guess these tests precede the gtest framework? I leave it up to others
> to decide what to do with them and to potentially move them out of
> metaspace.cpp.

Yes, ExecuteInternalVMTests preceeded gtest and these tests should be 
eventually moved, now that they can be.
> Side note: -XX:+ExecuteInternalVMTests seems to have bitrotted, see
> https://bugs.openjdk.java.net/browse/JDK-8202848. Does this get tested
> regularly?
>
> - metaspace.cpp is quite a bit smaller now - from ~5000 loc down to
> ~1700 loc. Arguably, one could split out more and clean up more, but I
> think this is a good start.

I think it's great.   I clicked on most but since it's just code 
movement, I reviewed the change.

Thanks,
Coleen

> ---
>
> I built locally on linux (release, fastdebug with and without pch,
> zero) and windows (64, 32bit). jdk-submit tests ran through with a
> build error on solaris sparc - I currently try to reproduce that build
> error with our very slow solaris machine.
>
> Thanks, Thomas