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

[Siebenborn, Axel]

Hi Thomas,
The change looks good .
I should have read your whole mail before starting looking at the changes, as you explain what you have done and what you didn't do ��.
Concerning the decision using ::metaspace vs. ::metaspace::internal:
I would use ::metaspace for the internal classes and leave the public classes in the global namespace.
This is more consistent to hotspot code where namespaces are rarely used (if this counts as an argument).
I would change that in this change, as having having ::metaspace::internal and nothing in ::metaspace seems to be the worst alternative. 

Altogether, this is really a good refactoring.

Cheers,
Axel

> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> bounces at openjdk.java.net] On Behalf Of Thomas Stüfe
> Sent: Montag, 14. Mai 2018 15:34
> To: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
> Subject: Re: RFR(L): 8176808: Split up metaspace.cpp
> 
> Ping...
> 
> Is there anything I can do to make review easier? This is really
> mostly code shuffling around (out of metaspace.cpp into new files), so
> with a bit of effort I could cook up some diffs for those parts.
> 
> ..Thomas
> 
> On Fri, May 11, 2018 at 8:31 AM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
> > All builds are fixed, jdk-submit tests ran through sucessfully.
> >
> > Latest webrev:
> > http://cr.openjdk.java.net/~stuefe/webrevs/8176808-split-metaspace-
> cpp/webrev.01/webrev/
> >
> > Only difference to the first webrev is the placement of a single
> > semicolon in occupancyMap.hpp, to satisfy the Solaris compiler.
> >
> > Thanks, Thomas
> >
> >
> >
> > On Wed, May 9, 2018 at 5:08 PM, Thomas Stüfe
> <thomas.stuefe at gmail.com> 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 ?
> >>
> >> 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.
> >> 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 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