New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8251158: Implementation of JEP 387: Elastic Metaspace #336
Conversation
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
Hi @openjdk[bot], thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user openjdk[bot] for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
Hi @openjdk[bot], thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user openjdk[bot] for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
Hi @mlbridge[bot], thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlbridge[bot] for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
/test |
Hi @openjdk[bot], thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user openjdk[bot] for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have a few cosmetic comments. Otherwise it looks good to me.
If you sort by ASCII order, I get a few unordered includes:
("." < "/" < "a-z,A-Z")
In file b/src/hotspot/share/gc/shared/genCollectedHeap.cpp:
[not sorted]:
#include "memory/metaspaceCounters.hpp" (Context)
#include "memory/metaspace/metaspaceSizesSnapshot.hpp" (Addition)
#include "memory/resourceArea.hpp" (Context)
In file b/src/hotspot/share/memory/metaspace.cpp:
[not sorted]:
#include "memory/metaspace/virtualSpaceList.hpp" (Context)
#include "memory/metaspace.hpp" (Addition)
#include "memory/metaspaceShared.hpp" (Context)
In file b/src/hotspot/share/memory/metaspace/chunkManager.cpp:
[not sorted]:
#include "memory/metaspace/metaspaceCommon.hpp" (Context)
#include "memory/metaspace/metaspaceContext.hpp" (Addition)
#include "memory/metaspace/metachunk.hpp" (Addition)
In file b/src/hotspot/share/memory/metaspace/chunkManager.cpp:
[not sorted]:
#include "memory/metaspace/metaspaceContext.hpp" (Addition)
#include "memory/metaspace/metachunk.hpp" (Addition)
#include "memory/metaspace/metaspaceSettings.hpp" (Addition)
blockTree.hpp
add a space after loop keyword "for(;;) {" -> "for (;;) {"
blockTree.cpp
add a space after loop keyword "} while(0)" -> "} while (0)" (twice in file)
I still think we should try to get the initializer list indented
somewhat consistently. (This is really boring, and hard as we do not
have precise indentation rules and no mechanical indenter). Sorry for
mentioning this, but now might be the time to get indentation
consistent at least in metaspace. The indentation level seems to most
often be 2 or 4. Sometimes "Haskell" indentation with "," at the
beginning of each line is used (I like this, but I think I am in a
small minority) and most often it is not used. Sometimes several field
members are initialised on multi line initializer lists, sometimes
only one per line (I prefer one per line, if not all fits on a single
line). Open curly braces seem to mostly (but not always) be on a new line.
If you could normalize the style I would be happy, I yield my own
preferences to you and the other reviewers on what style to use, so
that we have a possibility to agree :-)
Thanks, Leo
Hi @mlbridge[bot], thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlbridge[bot] for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
Regarding the order of "." and "/" take what you prefer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides my comment about globals.hpp option MetaspaceGuardAllocations, my comments are minor things and I approve this change. There might be some additional things we find that we'll want to change once this code is integrated. This is a significant improvement to metaspace memory management. Great work, @tstuefe !
Hi @openjdk[bot], thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user openjdk[bot] for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
Hi Leo,
I will fix all those.
Okay
I agree, will do. |
Hi, thanks for the reviews! New version:
XXX() : _m1(xx), _m2(xx) { XXX() :
|
Thank you Coleen! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far I have only reviewed the C code that uses the metaspace, but not the metaspace implementation itself (memory/metaspace*, memory/metaspace/*). I also reviewed a bit of classes.
@@ -1058,7 +1058,7 @@ void PSParallelCompact::post_compact() | |||
|
|||
// Delete metaspaces for unloaded class loaders and clean up loader_data graph | |||
ClassLoaderDataGraph::purge(/*at_safepoint*/true); | |||
MetaspaceUtils::verify_metrics(); | |||
DEBUG_ONLY(MetaspaceUtils::verify();) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be cleaner to declare MetaspaceUtils::verify() as
void verify() NOT_DEBUG_RETURN;
then you can omit the DEBUG_ONLY
at every caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you really want me to, or other Reviewers chime in, I'd rather leave it as it is. I prefer this style since it is clear at the callsites that this is only ASSERT code.
@@ -44,9 +44,8 @@ public static void main(String[] args) throws Exception { | |||
processArgs.add("-Xshare:dump"); | |||
|
|||
if (Platform.is64bit()) { | |||
processArgs.add("-XX:MaxMetaspaceSize=3m"); | |||
processArgs.add("-XX:MaxMetaspaceSize=8m"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean the absolute minimal size is larger than before? I just want to confirm this. I think 3M -> 8M doesn't really matter, unless other (larger) minimums also scale up by a factor of 2.66 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the contrary. Minimum metaspace usage to run a simple HelloWorld went way down.
Before: 10,00 MB reserved, 4,75 MB ( 48%) committed
Now: 12,00 MB reserved, 448,00 KB ( 3%) committed.
(CDS enabled in both cases)
mainly because now we commit the large chunk for the boot loader only on demand. Reservation (vsize) numbers are somwhat chunkier now since the default size for a VirtualSpaceNode went up from 4 to 8MB.
You now can get away with -XX:MaxMetaspaceSize=~500K to start a simple program.
While writing this, I wondered why I made this change to your test. It is not needed for the current version, probably a remnant from some earlier prototype. I will revert this line and check the other tests too.
test/hotspot/jtreg/runtime/cds/appcds/sharedStrings/LargePages.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Thomas,
this is batch 3 with another 3 points. I'm sending it now because I think I cannot reply to your comments otherwise.
Cheers, Richard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still approve this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Thomas,
this is a major enhancement and it looks good to me. Congratulations! :)
Cheers, Richard.
Thanks a lot Richard! |
Still looks good to me. |
/integrate |
Hi @openjdk[bot], thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user openjdk[bot] for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
Hi @openjdk[bot], thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user openjdk[bot] for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
Hi @openjdk[bot], thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user openjdk[bot] for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
@tstuefe Since your change was applied there have been 39 commits pushed to the master branch:
Your commit was automatically rebased without conflicts. Pushed as commit 7ba6a6b. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi all,
this is the continuation of the ongoing review for the JEP387 implementation (last rounds see [1] [2]). Sorry for the delay, had vacation then the entrance of Skara delayed things a bit.
For the delta diff please see [3].
This is the first time I do a large PR after Skara, so if something is wrong please bear with me. I cannot answer all feedback individually in this PR body, but I incorporated almost all into the new revision.
What changed since the last version:
I renamed most metaspace files back to the original naming scheme or to something similar, hopefully capturing the group consent.
I changed the way allocation guards are checked if MetaspaceGuardAllocations is enabled. Before, I would test for overwrites upon CLD destruction, but since that check was subject to VerifyMetaspaceInterval it only ran for every nth class loader which made it rather pointless. Now I run it always.
I also improved the printout on block corruption, and log block corruption unconditionally before asserting.
I also fixed up and commented the death test which tests for allocation overwriters (test_allocationGuard.cpp)
Side note, I find the corruption check very useful but if you guys think it is too much I still can remove the feature.
In ChunkManager::purge() I improved the comments after discussions with Leo.
I fixed a bug with VerifyMetaspaceInterval: if set to 1 the "SOMETIMES" sections were supposed to fire always, but due to a one-off error they only fired every second time. Now, if -XX:VerifyMetaspaceInterval=1, the checks really run every time.
Fixed indentation issues as Leo requested
Rewrote the condition and the assert in VirtualSpaceList::allocate_root_chunk() as Leo requested
I removed the "can_purge" logic from VirtualSpaceList. The list does not need to know. It just should iterate all nodes and attempt purging, and if a node does not own its ReservedSpace, it refuses to be purged. That is simpler and more flexible since it allows us to have list with purge-able and non-purge-able nodes.
and various smaller fixes, mainly on request of Leo.
@lkorinth:
You are right, _base and _word_size are directly related to the underlying space. But I'd prefer to leave it the way it is. Mainly because ReservedSpace::_base and ::_size are nonconst and theoretically can change under me. It is highly improbable but I'd like to know. Note that VirtualSpaceNode::verify checks that.
Should we clean up ReservedSpace at some point and make those members const - as they should be - then I would rewrite this as you suggest.
Thanks, again, for all your review work!
[1] https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041162.html
[2] https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-September/041628.html
[3] 731f795
/issue 8251158
/cc hotspot-runtime, hotspot-gc
/test tier1, tier2, tier3, tier4, tier5, tier6, tier7, tier8
Progress
Testing
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/336/head:pull/336
$ git checkout pull/336