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

Sat May 19 15:51:54 UTC 2018

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.

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