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

[Thomas Stüfe]

Hi Axel,

Thanks a lot for the review!

Remarks inline.

On Thu, May 17, 2018 at 12:15 PM, Siebenborn, Axel
<axel.siebenborn at sap.com> wrote:
> 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.

Yeah, I thought so too. I'll wait for other reviewers to chime in
though. I am fine either way.

>
> Altogether, this is really a good refactoring.

Thank you!

..Thomas

>
> 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