RFR: Implementation of JEP 387: Elastic Metaspace (round two)

Thomas Stüfe thomas.stuefe at gmail.com
Sat Sep 5 08:47:51 UTC 2020


Hi all,

This is Round Two of the review for JEP 387 "Elastic Metaspace". Please
find the first round of reviews here:
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041162.html

I did massage in all feedback - plus some smaller changes from me - and
here is the new version:

Full Webrev:
http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/webrev-all/webrev/
Patch applies cleanly atop of "8247352: improve error messages for sealed
classes and records"

Parts:
Core:
http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/webrev-core/webrev/
Test:
http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/webrev-test/webrev/
Misc:
http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/webrev-misc/webrev/

Delta webrev (somewhat useless due to many file renamings):
http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/delta.webrev/webrev/

List of fine granular commits since last webrev:
http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/changes_since_last_review.txt

Review guide (updated; added FAQ):
http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/guide/review-guide.pdf
http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/guide/review-guide.html

JEP: https://openjdk.java.net/jeps/387

------

What changed?

1) Aesthetics

Some of you requested style changes so there are a lot:

- Most files in memory/metaspace/ now follow a common theme with a common
prefix ("ms"). The only exception is metaspacesSizesSnapshot.(cpp|hpp)
which I plan to remove in a follow up (see JDK-8251342).
- Files in gtest/metaspace have also been partly renamed to be more
consistent
- Did rename enums, members and structures to adhere to hotspot C++ style
guide
- Provided constructors for most structures
- Did use placement new where appropriate
- Switched to enum class where possible. Note though that enum classes do
not allow (easily) to iterate over its values, therefore I cannot use it
for MetaspaceType, MetadataType.
- Removed metaspaceEnum.cpp/hpp and moved all those helper functions back
to metaspace.cpp/hpp. I also renamed them back to their original names
(e.g. metaspace::is_class() -> Metaspace::is_class_space_allocation()) to
reduce the total patch size. These renamings will show up in the delta diff
though.
- gtest: Did remove all includes from metaspaceTestCommon.hpp and moved
them to the individual cpp files as Coleen suggested
- Fixed include guard names and copyrights
- Whitespace- and empty-line-cleanup

http://hg.openjdk.java.net/jdk/sandbox/rev/018c370bbbd5  Rename static
constants according to naming laws
http://hg.openjdk.java.net/jdk/sandbox/rev/d509e2c607c7
 MetaspaceReporter::ReportFlag: rename and make enum class
http://hg.openjdk.java.net/jdk/sandbox/rev/b029ae20f238  Metachunk::state_t
rename and enum class
http://hg.openjdk.java.net/jdk/sandbox/rev/8e285bdfcca1  Correct formatting
(tabs)
http://hg.openjdk.java.net/jdk/sandbox/rev/a14ba4a2fb8c  Remove superfluous
spaces
http://hg.openjdk.java.net/jdk/sandbox/rev/223c19deea53  Add constructors
to structs
http://hg.openjdk.java.net/jdk/sandbox/rev/0bf8e94f9e09  Rename struct
members to follow naming laws
http://hg.openjdk.java.net/jdk/sandbox/rev/29513e1f6d49  Remove unused
struct BitCounterClosure
http://hg.openjdk.java.net/jdk/sandbox/rev/7facf1afd137  Wholesale rename
all structs to adhere to naming laws
http://hg.openjdk.java.net/jdk/sandbox/rev/d09f635f3a07  Wholesale file
renamings <<< Thats the big rename change <<<
http://hg.openjdk.java.net/jdk/sandbox/rev/a56e69aba3b1  Fix include guard
names in gtest headers
http://hg.openjdk.java.net/jdk/sandbox/rev/788f6454226f  Fix copyrights in
new gtest sources
http://hg.openjdk.java.net/jdk/sandbox/rev/bcb4bbc71cc9  Remove
metaspaceEnums.cpp,hpp
http://hg.openjdk.java.net/jdk/sandbox/rev/a3d33ce1b9db  Rename
FreeBlocks::_v to _blocks
http://hg.openjdk.java.net/jdk/sandbox/rev/fe5aa08178d3  Fix include guard
names
http://hg.openjdk.java.net/jdk/sandbox/rev/1e89f4dfd8a7  Remove empty lines
http://hg.openjdk.java.net/jdk/sandbox/rev/f7bf12bdbfe4  Gtests: consistent
naming of context instances
http://hg.openjdk.java.net/jdk/sandbox/rev/551226c61470  Beautify comment
for MetaspaceArena


2) Changes to BlockTree and friends

Both Leo and Richard reviewed the free block management in detail. Changes:

- Made all BlockTree functions non-recursive.
- Did remove offending typedefs (BinListXX etc)
- Richard found two bugs (BlockTree::find_closest_fit,
BlockTree::remove_from_list); I fixed those and wrote regression gtests
(see test_blocktree.cpp)
- In BlockTree::insert, renamed "forebear" argument to "insertion_point"
- Did consistently rename "get_block" to "remove_block" in all classes
- Tweaked gtests for BlockTree and BinList to make them clearer
- Did remove the "splinter threshold" logic in FreeBlocks; that logic was
used to determine when chopping off remainder space from a returned block
was worth the effort (see FreeBlocks::remove_block()). But it is always
worth doing, so no need for this threshold.
- Did remove, from BlockTree, the "largest block added" logic since Leo
convinced me it was less useful than I thought.

http://hg.openjdk.java.net/jdk/sandbox/rev/78a34af45cb8  make
BlockTree::print_tree non-recursive
http://hg.openjdk.java.net/jdk/sandbox/rev/00246785a20e  Grooming BlockTree
(and various unrelated fixes)
http://hg.openjdk.java.net/jdk/sandbox/rev/24ed785d5c51  Simplify, comment
BlockTree_basic_siblings test
http://hg.openjdk.java.net/jdk/sandbox/rev/e1df0dbc7cc9  Fix
BlockTree::find_closest_fit and add test
http://hg.openjdk.java.net/jdk/sandbox/rev/0284f4705973  Rename Forebear
http://hg.openjdk.java.net/jdk/sandbox/rev/4340648bd624  Remove BinList8,
BinList16, BinList64, SmallBlocksType typedef
http://hg.openjdk.java.net/jdk/sandbox/rev/7c74250c35fd  Clarify BlockTree
gtest
http://hg.openjdk.java.net/jdk/sandbox/rev/4680ad0ae1db  Remove
largest-block-added optimization from BlockTree
http://hg.openjdk.java.net/jdk/sandbox/rev/255d1c34356f  FreeBlocks,
BinList, BlockTree: code grooming
http://hg.openjdk.java.net/jdk/sandbox/rev/e07c51de3056  Remove unused
splinter_threshold from FreeBlocks

3) Coleen asked me to remove the gtest for Metaspace reporting, and instead
to provide a jtreg test. I then found that I had written bearish such a
test already, and just expanded it a bit. I also re-enabled it for ZGC, for
some reason it had been disabled.

http://hg.openjdk.java.net/jdk/sandbox/rev/abba106ec3c0  Extend cases for
PrintMetaspaceDcmd

4) Did some code grooming for allocation guards, nothing major, and added a
new gtest, using the newly discovered assertion test feature :), to test
that we notice overwriters in metaspace:

http://hg.openjdk.java.net/jdk/sandbox/rev/07089d3e4be4  Reform allocation
guard coding and add gtest

5) I did beef up the gtests for chunk enlargement, and made them easier to
read:

http://hg.openjdk.java.net/jdk/sandbox/rev/8fe76f9ad8ee  Improve/extend
testing for chunk enlargement

6) Did remove a number of dead code sections

http://hg.openjdk.java.net/jdk/sandbox/rev/29513e1f6d49  Remove unused
struct BitCounterClosure
http://hg.openjdk.java.net/jdk/sandbox/rev/50bfadc554cd  Remove dead code
in metaspaceContext.cpp
http://hg.openjdk.java.net/jdk/sandbox/rev/f00879c58720  Remove dead gtest
files
http://hg.openjdk.java.net/jdk/sandbox/rev/074a374e9c5c  Remove a dead
portion of code
http://hg.openjdk.java.net/jdk/sandbox/rev/b10cf8275580  Remove setting
which had been effectively unused.

7) I removed the "slow" parameter from all "::verify()" methods since it
was not that useful. Expensive code section I place now inside a SOMETIMES
clause which executes the designated code sometimes, at regular intervals,
controlled via VerifyMetaspaceInterval (e.g. -XX:VerifyMetaspaceInterval=1
will execute the code always. That way we don't pay the full performance
loss for these tests but still run them occasionally.

http://hg.openjdk.java.net/jdk/sandbox/rev/f18d566c5875  Rework
xxx::verify() functions

8) While testing on 32bit I found an error in destruction of
MetaspaceTestContext  where I forgot to unmap the ReservedSpace for the
space-provided-from-outside case (which simulates ccs):

http://hg.openjdk.java.net/jdk/sandbox/rev/d4f358b658a5 clean up spaces for
non-expandable test contexts

-----

Follow up items:

I refrained from doing too large changes to not disturb the review
process, and to keep the patch stable. Changes which are not strictly
necessary but maybe a good idea I collect in follow up issues to be done
once this patch is upstream:

https://bugs.openjdk.java.net/browse/JDK-8251342 "Rework JFR metaspace free
chunk statistics after JEP 387"
https://bugs.openjdk.java.net/browse/JDK-8251392 "Brush up and consolidate
Metaspace statistics after JEP 387"
https://bugs.openjdk.java.net/browse/JDK-8252014 "Find a better place for
counter utility classes after JEP387"
https://bugs.openjdk.java.net/browse/JDK-8252132 "Investigate
MetaspaceArena locking after JEP387"
https://bugs.openjdk.java.net/browse/JDK-8252187 "Optimize freeblocks
storage in MetaspaceArena after JEP387"
https://bugs.openjdk.java.net/browse/JDK-8252189 "Clarify meaning of OOM
texts for out-of-metaspace errors"

-----

Thanks alot for your review work! I know it is hard, and it is very
appreciated. I hope we now got all across-the-board style changes done, so
the next delta diff will be easier to read.

Also, feel free to contact me in case you have quick questions, or for a
quick zoom meeting should that be easier. Please note that starting today I
will have vacation but should be back by mid September.

Cheers, Thomas


More information about the hotspot-runtime-dev mailing list