-
Notifications
You must be signed in to change notification settings - Fork 224
8256844: Make NMT late-initializable #496
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
Conversation
Reviewed-by: coleenp, zgu
👋 Welcome back vkempik! A progress list of the required criteria for merging this PR into |
This backport pull request has now been updated with issue from the original commit. |
I'm all for this and had planned to do this sometime this year, but please be aware that this is a bit larger and needs to be tested thoroughly. So far it has seen the light of customer machines only for JDK18, so probably not too much exposure there. At the very least we should and will cook this patch for a week or two in our JDk17 CI. Would be nice if you could schedule some tests too. Cheers, Thomas |
BTW getting NMT to run under IntelliJ was one of the reasons for this patch :) |
That is my main objective, Since idea 2022.2 is migrating to jdk17. |
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.
Test changes are okay.
I'd like to see some mass tests running though, for some days maybe. I'm worried about the changed pre-init allocation mechanics. This patch has been well tested in 18 and beyond, so I may just be paranoid. Just worry about aspects I did not think to test, like native integration.
One small concern I have is that with this patch, we stop recognizing the old NMT_LEVEL_<pid>
variable. This needs a patch note somewhere (no clue where, @GoeLin ?).
@VladimirKempik 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 10 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 |
Github CI testing is ok |
Cherry pick of PR openjdk/jdk17u-dev#496 Reviewed-by: stuefe
Testing showed no problems for several days. Good from my end. |
I'll wait a bit more for tests to complete in JB env. |
Cherry pick of PR openjdk/jdk17u-dev#496 Reviewed-by: stuefe
Tests look good in JB infra too |
/integrate |
Going to push as commit 774213f.
Your commit was automatically rebased without conflicts. |
@VladimirKempik Pushed as commit 774213f. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review this backport to jdk17u-dev
Doesn't apply clean because of JDK-8280940 backport which introduced the same patch into test_os.cpp
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev pull/496/head:pull/496
$ git checkout pull/496
Update a local copy of the PR:
$ git checkout pull/496
$ git pull https://git.openjdk.org/jdk17u-dev pull/496/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 496
View PR using the GUI difftool:
$ git pr show -t 496
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/496.diff