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

[David Holmes]

On 20/05/2018 1:51 AM, Thomas Stüfe wrote:
> Hi Coleen,
> 
> On Sat, May 19, 2018 at 3:20 PM, <coleen.phillimore at oracle.com> wrote:
> 
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8176808-split-
>> metaspace-cpp/webrev.02/webrev/src/hotspot/share/memory/metaspace/
>> metaspaceCommon.hpp.udiff.html
>>
>> Not your change but metaspaceCommon.hpp has these with a trailing _ ?
>>
>> #ifndef SHARE_MEMORY_METASPACE_METASPACECOMMON_HPP_
>>   #define SHARE_MEMORY_METASPACE_METASPACECOMMON_HPP_
>>
>> The other new files have it this too.  I don't think we have this anywhere
>> else and it looks strange.  I don't like the _.
>>
>>
> Sure, I remove it. These lines were autogenerated by CDT. Maybe I should
> return to good old vi :)
> 
> 
>> http://cr.openjdk.java.net/~stuefe/webrevs/8176808-split-
>> metaspace-cpp/webrev.02/webrev/src/hotspot/share/memory/metaspace/
>> virtualSpaceList.cpp.html
>>
>> This is missing a Copyright.  Are you supposed to put the SAP line on
>> these new files also?
>>
> 
> I usually place SAP copyrights in addition to Oracle ones on those files
> fully created by me.
> 
> 
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8176808-split-
>> metaspace-cpp/webrev.02/webrev/src/hotspot/share/
>> memory/metaspace/occupancyMap.cpp.html
>>
>>     3  * Copyright (c) 2018, SAP.
>>
>>
>> Is this SAP copyright correct?  I thought there was more on the line.
>> Maybe not.
>>
> 
> I'll double check on Tuesday with my coworkers. jdk-submit seems to be dead
> currently anyway, and I want to put the fix one last time thru the checks.

The format seems to be e.g.:

  * Copyright (c) 2018 SAP SE. All rights reserved.

Thanks,
David

> 
>>
>> For the _ changes and copyright change, I don't need to see another
>> webrev.  This refactoring and the use of namespace metaspace is really
>> nice.  It does give an appreciation for how complicated the memory
>> management for metaspace became since its start as:
>>
>> class Metaspace : public Arena {}.
>>
>>
> hah! yes indeed :)
> 
> But as I mentioned before, my suspicion is that were we to recreate it from
> scratch it would soon grow in complexity until something close to the
> current form. Maybe not, but I would not be surprised.
> 
> 
>> Thanks!
>>
> 
> Sure. Thanks for reviewing!
> 
> ..Thomas
> 
> 
>>
>> Coleen
>>
>>
>>
>> On 5/19/18 12:55 AM, Thomas Stüfe wrote:
>>
>> 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 ��.
>>>> 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
>>>>>>>
>>>>>>
>>>
>>
>>