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

Tue Sep 8 21:41:19 UTC 2020

On 9/5/20 4:47 AM, Thomas Stüfe wrote:
> 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).

I really don't like the ms prefix on all the files at all.  I didn't 
think there were any conflicting names in the metaspace directory but if 
there were, they could be renamed.  Now the class names don't match the 
file names!  The other thing about sort of generic names in the metspace 
directory, even if they don't match other names in the system, like 
binList.hpp or blockTree.hpp, is that they can provide a hint to people 
before they write similar code.  These could be a basis for 
generalization into the utilities directory if possible.

http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/webrev-all/webrev/src/hotspot/share/memory/metaspace/msContext.cpp.html

For example in this case, the class name is MetaspaceContext which is a 
much better name than MSContext.

Sorry I didn't object to this sooner on the thread.

More scattered comments:

http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/webrev-all/webrev/src/hotspot/share/gc/shared/genCollectedHeap.cpp.udiff.html

Why is MetaspaceSizesSnapshot included?

http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/webrev-all/webrev/src/hotspot/share/memory/metaspace/msAllocationGuard.hpp.html

This file defines a class called Prefix.  Maybe this could be Metaprefix 
to follow the (somewhat overused) Meta<lower case letter>word naming 
convention.

http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/webrev-all/webrev/src/hotspot/share/memory/metaspace/msArena.cpp.html

The renaming of SpaceManager to MetaspaceArena is one of my favorite 
things about this change.

http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/webrev-all/webrev/src/hotspot/share/memory/metaspace/msChunklevel.hpp.html

I find this namespace nesting and naming inconsistent.  Can you just 
make this ChunkLevel an AllStatic class?   And if it's too generic, it 
could be MetachunkLevel instead.

http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/webrev-all/webrev/src/hotspot/share/memory/metaspace/msPrintMetaspaceInfoKlassClosure.hpp.html
http://cr.openjdk.java.net/~stuefe/jep387/review-2020-09-04/webrev-all/webrev/src/hotspot/share/memory/metaspace/msPrintCLDMetaspaceInfoClosure.hpp.html

Can you just include this code into the files that use it?  The closure 
seems pretty specific to the caller (4 less files).

Besides the naming, I don't see anything major anymore that would 
prevent me from hitting approved in 'git' when you send your pull request.

thanks,
Coleen
> - 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
>
>
>
>
>