-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8368781: PerfMemory - make issues more transparent #27602
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
I think adding a new diagnostic flag named |
Might be an option ; PrintMiscellaneous is used besides perfMemory also at some more places , e.g. C2 JIT.
Would it be okay to offer this in product JVMs ? (Verbose is even harder to say, because it is used more) |
PerfMemory should be converted to Unified Logging (UL). We don't want any new flags for ad-hoc logging. |
Guess this makes sense, do you have some special UL category in mind? Maybe 'os' ? Or a new one ? |
We already have some UL in PerfMemory:
but for some reason that effort completely ignored the PrintMiscellaneous/Verbose "warnings". To keep things simple I suggest the following pattern:
Of course you proposed changes as a potential consumer of this information, so how would you like to procure it? |
I think my support colleague would set the needed UL flags in case her wants to find out more about perfmem issues. |
@dholmes-ora while looking into the Windows perfmem code, I noticed we are using SetSecurityDescriptorControl jdk/src/hotspot/os/windows/perfMemory_windows.cpp Line 1016 in 1653999
but we use still handling for ancient Windows versions (we use dynamic handling of the function at runtime), should we remove this ? |
Do we need to include more? It seems not to work in e.g. the zero of minimal builds ? |
Yes please file a separate JBS issue for that |
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.
You need to include:
#include "logging/logStream.hpp"
to get LogStream
. But note that simple cases don't need to use LogStream
. I gave that example for the more complex cases.
Thanks
LogStreamHandle(Debug, perf) log; | ||
log.print_cr("Could not commit PerfData memory\n"); |
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.
For simple usages like this you can simplify.
LogStreamHandle(Debug, perf) log; | |
log.print_cr("Could not commit PerfData memory\n"); | |
log_debug(perf)("Could not commit PerfData memory"); |
also note to get rid of the explicit \n
in the strings.
I created |
Currently issues with perfMemory like problems with the secure tmp subdirectory creation are not very transparent in release JVMs.
There exists some warnings traces but they are behind develop flags like Verbose so only available in debug JVMs.
We could (in case of issues) store some information and write it later into hsinfo/hserr files ; or make the existing warnings available too in release JVMs.
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27602/head:pull/27602
$ git checkout pull/27602
Update a local copy of the PR:
$ git checkout pull/27602
$ git pull https://git.openjdk.org/jdk.git pull/27602/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27602
View PR using the GUI difftool:
$ git pr show -t 27602
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27602.diff
Using Webrev
Link to Webrev Comment