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

[coleen.phillimore at oracle.com]

On 5/17/18 6:15 AM, Siebenborn, Axel 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.

Yes, I agree with this.  One namespace to encapsulate things like 
ChunkManager and SpaceManager and these things is good, without adding 
the another nested "internals" namespace.  That's too much for us :)

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