-
Notifications
You must be signed in to change notification settings - Fork 722
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
Add Unmounted Thread info to javacore #18218
Conversation
@babsingh @TobiAjila Do you have any recommendation on what option should this be en-able under? |
Option 1: Add Option 2: Add a standalone option. Existing option: @pshipton Requesting your opinion. Shall we proceed with Option 2? |
Option 2 is fine with me. What is the downside of enabling this by default? |
This can create very large javacores in case of stress benchmarks, which can have 1 million+ virtual threads. In extreme cases, this can cause disk space issues on machines (jenkins) and artifactory. We should selectively enable this option for diagnosis where the user can monitor the size of the javacores and perform the required clean up at the end. |
Option 2 looks good |
178333f
to
edb2029
Compare
sample output in javacore dump:
@babsingh can you please take a look |
Had an in-person review with @fengxue-IS. Few doubts about the high level design and minor formatting nitpicks. @fengxue-IS to clarify design requirements for the new feature with @tajila. |
Per discussion with @babsingh, few points to clarify:
|
I imagine the eyecatchers like |
I think the defensive option is to name it differently and tools can opt in to collecting that info. |
In general I think anything that is mounted should be printed as part of the standard javacore output. So in the case where I think it makes sense to separate Thread details from Virtual Threads as tools may already know about former. So for the following:
Separate section
In thread details we should print the carrier stack. In the Virtual Thread section we should print the vthread stack. This implies that if there is a single vthread mounted, we will have a VirtualThread section regardless of what options are passed.
If we do the above, I think we can avoid this issue We can discuss tomorrow to see how feasible this approach is. |
Proposed new format changes:
ex:
|
684e69c
to
c4e3264
Compare
@fengxue-IS Can you please post some sample output for what the latest changes look like with mounted and unmounted threads? |
FYI @tajila |
Im running personal builds to verify the change, will mark PR ready once testing have passed. |
personal build & local testing passed. @keithc-ca can you take another look please. |
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.
Please make the requested changes.
Updated code per review comments. |
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.
Please squash.
Refactor RasDumpGlobalStorage.showNativeSymbols field into a bit flag dumpFlags Add new J9RAS_JAVADUMP_SHOW_UNMOUNTED_THREAD_STACKS bit flag Add VThread info to javacore - Fix writeThreadName API to correctly reflect mounted virtual thread name - Add new tag for virtual thread related info - Move get VThread name code inside j9sig_protect call Signed-off-by: Jack Lu <[email protected]>
Jenkins test sanity alinux jdk17,jdk21 |
Please create a docs issue. Ideally we would also have some tests that exercise this and openj9.dtfj should be able to parse the new information (see |
All the test failures are related to CRIU, not due to 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.
LGTM. @tajila's approval is pending before this can be merged.
jenkins test sanity alinux64 jdk21 |
@fengxue-IS we should rtry to get this into 0.43 |
It's after M2. If it causes any regressions we may not catch them before RC1. If we can get this merged before the weekend that would help, if the Semeru extended testing runs. |
We should have a docs issue opened for this. |
Docs issue have been opened at eclipse-openj9/openj9-docs#1241, |
Since it's disabled by default it should be low risk to include in 0.43 |
Add -XX:[+|-]ShowUnmountedThreadStacks option
Refactor RasDumpGlobalStorage.showNativeSymbols field into a bit flag dumpFlags
Add new J9RAS_JAVADUMP_SHOW_UNMOUNTED_THREAD_STACKS bit flag
Signed-off-by: Jack Lu [email protected]