-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
JDK-8275608: runtime/Metaspace/elastic/TestMetaspaceAllocationMT2 too slow #6041
JDK-8275608: runtime/Metaspace/elastic/TestMetaspaceAllocationMT2 too slow #6041
Conversation
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
Webrevs
|
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.
Looks good to me; you might want to update the copyright headers too (no new review needed for that of course).
@tstuefe This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 3 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Thank you Matthias! Will update the copyright before pushing. |
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.
Looks fine to me, some minor nits I wondered about below.
test/hotspot/jtreg/runtime/Metaspace/elastic/RandomAllocator.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/runtime/Metaspace/elastic/RandomAllocator.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/runtime/Metaspace/elastic/MetaspaceTestManyArenasManyThreads.java
Show resolved
Hide resolved
Co-authored-by: Aleksey Shipilëv <shade@redhat.com>
Co-authored-by: Aleksey Shipilëv <shade@redhat.com>
@@ -41,7 +41,7 @@ | |||
*/ | |||
|
|||
/* | |||
* @test id=debug-default | |||
* @test id=debugdefault |
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.
Why these changes?
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.
Nevermind, got my reply.
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.
As I wrote, I changed the test names, because I want to be able to call them individually with '#bla' and that does not work if the subtest name contains non-alphanumerics. I can do it in a separate RFE if 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.
Yes, I don't mind doing it here.
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 removed the renaming from this patch, its maybe cleaner to do it separately. Or, idk, maybe fix jtreg itself.
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.
Fixing jteg would be nice. There are plenty of tests with these dashed IDs, IIRC.
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.
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.
Not a committer to code-tools though. I'll need a sponsor.
Changes:
I also changed the test names, because I want to be able to call them individually with '#bla' and that does not work if the subtest name contains non-alphanumerics. |
Going to push as commit d6d82f5.
Your commit was automatically rebased without conflicts. |
Small tweaks to make these elastic metaspace stress tests faster.
Before, runtime for individual tests was wobbling between 40+ seconds and some rare timeouts at 3+ minutes, depending on machine load and random luck. Now, we get a steadfast 35-37 seconds.
I tested this manually, checking the numbers that we really test what we want to test (a lot of arena activity). Also GHAs and SAP nightlies.
Thanks, Thomas
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6041/head:pull/6041
$ git checkout pull/6041
Update a local copy of the PR:
$ git checkout pull/6041
$ git pull https://git.openjdk.java.net/jdk pull/6041/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 6041
View PR using the GUI difftool:
$ git pr show -t 6041
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6041.diff