RFR(L): 8176808: Split up metaspace.cpp
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Sat May 19 13:20:29 UTC 2018
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 _.
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?
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.
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 {}.
Thanks!
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/
> <http://cr.openjdk.java.net/%7Estuefe/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/
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8176808-split-metaspace-cpp/webrev.02/webrev/>
>
> Thanks, Thomas
>
>
> On Sat, May 19, 2018 at 12:41 AM, <coleen.phillimore at oracle.com
> <mailto: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-
> <mailto:hotspot-runtime-dev->
> bounces at openjdk.java.net
> <mailto: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
> <mailto: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 <mailto: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-
> <http://cr.openjdk.java.net/%7Estuefe/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 <mailto: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
> <https://bugs.openjdk.java.net/browse/JDK-8176808>
> Webrev:
> http://cr.openjdk.java.net/~stuefe/webrevs/8176808-split-
> <http://cr.openjdk.java.net/%7Estuefe/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
> <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
>
>
>
More information about the hotspot-runtime-dev
mailing list