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

[Thomas Stüfe]

Hi, Coleen and Axel,

thanks a lot for the reviews! I will be very happy once this is removed
from my patch queue, since merging the patch is becoming a pain.

New webrev.

As suggested, I removed the "internals" inner namespace, which leaves us
with "metaspace::". Nothing else changed. Still fine?

Webrevs:
Incremental:
http://cr.openjdk.java.net/~stuefe/webrevs/8176808-split-metaspace-cpp/webrev.01-to-02/webrev/
Full:
http://cr.openjdk.java.net/~stuefe/webrevs/8176808-split-metaspace-cpp/webrev.02/webrev/

Thanks, Thomas


On Sat, May 19, 2018 at 12:41 AM, <coleen.phillimore at oracle.com> wrote:

>
>
> 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 [image:
>> ��].
>> 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
>>>>>
>>>>
>