-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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-8312525: New test runtime/os/TestTrimNative.java#trimNative is failing: did not see the expected RSS reduction #14984
Conversation
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
511b2aa
to
69837cc
Compare
69837cc
to
d6a4a09
Compare
0f5bc3b
to
28537f2
Compare
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.
Seems reasonable. Not 100% sure about the extra logging ... one query in relation to that.
Thanks.
src/hotspot/os/linux/os_linux.cpp
Outdated
// Log also: | ||
// - LD_PRELOAD, since this is the most popular mechanism to divert libc functions |
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.
What about the content of /etc/ld.so.preload? (We show that in hs_err file.)
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.
Oh, you are right. I'll check how much more lines that would add; I don't want to bloat the complexity too much.
@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 45 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@dholmes-ora : I removed parts of the logging. I went down a small rabbit hole about extending to /etc/ld.preload, about unifying that code with existing ld_preload printing, about security concerns (maybe we should obfuscate the values when logging), then I gave up and just removed it, since it does not directly relate to the patch at hand. |
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.
Ok
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.
Thanks @dholmes-ora and @shipilev, and thanks for running this through Oracles CI! /integrate |
Going to push as commit ad34be1.
Your commit was automatically rebased without conflicts. |
The test causes intermittent rare errors, so far only seen on Oracle 7.9 with glibc 2.17.
This PR disables, for automated testing, the RSS evaluation part of the test. It retains the mechanical part (testing whether trims happen at all, in the expected number). The stricter variant of the test is kept as manual tests (usable with jtreg without -a).
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14984/head:pull/14984
$ git checkout pull/14984
Update a local copy of the PR:
$ git checkout pull/14984
$ git pull https://git.openjdk.org/jdk.git pull/14984/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14984
View PR using the GUI difftool:
$ git pr show -t 14984
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14984.diff
Webrev
Link to Webrev Comment