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

Mon May 21 09:36:09 UTC 2018

I pushed, and before that I changed the include guard names and the
copyright headers as proposed.

Thanks a lot, Thomas

On Mon, May 21, 2018 at 4:11 AM, David Holmes <david.holmes at oracle.com>
wrote:

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