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

[Thomas Stüfe]

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