Skip to content

Conversation

@lmesnik
Copy link
Member

@lmesnik lmesnik commented Nov 15, 2024

Hi
Could you please review the the fix that add locks information into SA jhsdb stack --mixed output.

Here is the motivation for this rfe and explanation why I add it into SA now.

The information about current owners of Hotspot Mutex is often very useful for dealock investigations.

The jcmd usually doesn't work because VM can't reach or exit safepoints. So it doesn't respond to jcmd.

The SA 'jstack --mixed' provides information about stacktraces on Java and non-Java Threads. So having information about locks along with stack traces might significantly help to identify issues.

The gdb allows to provide stacktraces, but the debug symbols are required to get info about locks. These symbols are often absent during execution.
Also the debugger solution is OS specifc.

The significant part of fix is refacotorrng of mutex_array to be vmStructs compatible.

The adding support of non-JavaThreads into SA might be implemented later to obtain more info about their names.
The example of output:

[2024-11-06T21:32:48.897414435Z] Gathering output for process 1620563
Attaching to process ID 1620533, please wait...
Debugger attached successfully.
Server compiler detected.
JVM version is 24-internal-adhoc.lmesnik.open
Deadlock Detection:

No deadlocks found.

Internal VM Mutex Threads_lock is owned by Unknnown thread (Might be non-Java Thread) with address: 0x00007f8cf825b190
Internal VM Mutex Compile_lock is owned by LockerThread with address: 0x00007f8cf8309a00
----------------- 1620559 -----------------
"C1 CompilerThread4" #28 daemon prio=9 tid=0x00007f8c300566a0 nid=1620559 runnable [0x0000000000000000]
java.lang.Thread.State: RUNNABLE
JavaThread state: _thread_blocked
0x00007f8cff11e88d syscall + 0x1d
0x00007f8cfe6c99de LinuxWaitBarrier::wait(int) + 0x8e
0x00007f8cfe2be409 SafepointMechanism::process(JavaThread*, bool, bool) + 0x79
0x00007f8cfd53ea91 ThreadInVMfromNative::ThreadInVMfromNative(JavaThread*) + 0xc1
0x00007f8cfd534a00 ciEnv::cache_jvmti_state() + 0x30
0x00007f8cfd679614 CompileBroker::invoke_compiler_on_method(CompileTask*) + 0x204
0x00007f8cfd67adc8 CompileBroker::compiler_thread_loop() + 0x5c8
0x00007f8cfdb4426c JavaThread::thread_main_inner() + 0xcc
0x00007f8cfe5a0bbe Thread::call_run() + 0xbe
0x00007f8cfe16813b thread_native_entry(Thread*) + 0x12b
.....
----------------- 1620554 -----------------
"LockerThread" #31 prio=5 tid=0x00007f8cf8309a00 nid=1620554 waiting on condition [0x00007f8cc1dfe000]
java.lang.Thread.State: RUNNABLE
JavaThread state: _thread_blocked
0x00007f8cff091117 __GI___futex_abstimed_wait_cancelable64 + 0xe7
0x00007f8cff093a41 __GI___pthread_cond_wait + 0x211


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8343741: SA jstack --mixed should print information about VM locks (Enhancement - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22171/head:pull/22171
$ git checkout pull/22171

Update a local copy of the PR:
$ git checkout pull/22171
$ git pull https://git.openjdk.org/jdk.git pull/22171/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22171

View PR using the GUI difftool:
$ git pr show -t 22171

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22171.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 15, 2024

👋 Welcome back lmesnik! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Nov 15, 2024

@lmesnik 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:

8343741: SA jstack --mixed should print information about VM locks

Reviewed-by: cjplummer

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 165 new commits pushed to the master branch:

  • 1b2d9ca: 8344881: Problemlist java/awt/Robot/InfiniteLoopException.java on Linux
  • 6aec2dc: 8344788: Specify that the access control context parameters of Subject.doAsPrivileged are ignored
  • 079f503: 8344568: Renaming ceil_log2 to log2i_ceil
  • 51763b6: 8344525: Fix leftover ExceptionOccurred in java.base
  • 4b16530: 8344795: Remove uses of AccessControlContext in java.desktop module
  • 5154b71: 8343598: Since Checker can mark some preview elements as new even if bytecode reference is identical
  • 8b98f95: 8298387: Implement JEP 497: Quantum-Resistant Module-Lattice-Based Digital Signature Algorithm
  • 21e0fb8: 8343529: serviceability/sa/ClhsdbWhere.java fails AssertionFailure: Corrupted constant pool
  • 13987b4: 8298390: Implement JEP 496: Quantum-Resistant Module-Lattice-Based Key Encapsulation Mechanism
  • 6d3becb: 8344861: Disable CheckJNICalls in tests until JDK-8344802 is fixed
  • ... and 155 more: https://git.openjdk.org/jdk/compare/c388455d0a463c9cb52ad18050f1155ec4ac0e6c...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 15, 2024
@openjdk
Copy link

openjdk bot commented Nov 15, 2024

@lmesnik The following labels will be automatically applied to this pull request:

  • hotspot
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@mlbridge
Copy link

mlbridge bot commented Nov 15, 2024

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of drive-by comments/suggestions.

The functionality seems like a reasonable addition.

Thanks

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updates. svc folk will need to do actual Review though.

@plummercj
Copy link
Contributor

The significant part of fix is refacotorrng of mutex_array to be vmStructs compatible.

Can you explain what the issue was? For the most part what I can tell is you moved a lot of code from MutexLocker to Mutex, but the reason isn't clear.

@lmesnik
Copy link
Member Author

lmesnik commented Nov 20, 2024

The significant part of fix is refacotorrng of mutex_array to be vmStructs compatible.

Can you explain what the issue was? For the most part what I can tell is you moved a lot of code from MutexLocker to Mutex, but the reason isn't clear.

IIUC The VMStructs can read classes only, so I moved _mutex_array to be a static member of Mutex. All other refactoring just needed for this change.

@plummercj
Copy link
Contributor

The significant part of fix is refacotorrng of mutex_array to be vmStructs compatible.

Can you explain what the issue was? For the most part what I can tell is you moved a lot of code from MutexLocker to Mutex, but the reason isn't clear.

IIUC The VMStructs can read classes only, so I moved _mutex_array to be a static member of Mutex. All other refactoring just needed for this change.

Ok, that makes sense. I remember now you mentioning to me a couple of weeks ago that SA can't access globals via VMStructs.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 21, 2024
@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Nov 21, 2024
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 21, 2024
@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Nov 21, 2024
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 21, 2024
Copy link
Member

@stefank stefank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of putting this inside of Mutex, could you instead create a class that is the container of this array? I think that would make a cleaner split between the code that operates on a specific Mutex vs code that is dealing with the set of registered Mutexes. Maybe name it MutexSet, MutexList, MutexArray, RegisteredMutexes, ...?

If you don't like that suggestion I'd prefer if you fixed the nits I mention below.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Nov 21, 2024
Copy link
Member

@stefank stefank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple more nits.

@lmesnik
Copy link
Member Author

lmesnik commented Nov 22, 2024

/integrate

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 22, 2024
@openjdk
Copy link

openjdk bot commented Nov 22, 2024

Going to push as commit 98b6678.
Since your change was applied there have been 165 commits pushed to the master branch:

  • 1b2d9ca: 8344881: Problemlist java/awt/Robot/InfiniteLoopException.java on Linux
  • 6aec2dc: 8344788: Specify that the access control context parameters of Subject.doAsPrivileged are ignored
  • 079f503: 8344568: Renaming ceil_log2 to log2i_ceil
  • 51763b6: 8344525: Fix leftover ExceptionOccurred in java.base
  • 4b16530: 8344795: Remove uses of AccessControlContext in java.desktop module
  • 5154b71: 8343598: Since Checker can mark some preview elements as new even if bytecode reference is identical
  • 8b98f95: 8298387: Implement JEP 497: Quantum-Resistant Module-Lattice-Based Digital Signature Algorithm
  • 21e0fb8: 8343529: serviceability/sa/ClhsdbWhere.java fails AssertionFailure: Corrupted constant pool
  • 13987b4: 8298390: Implement JEP 496: Quantum-Resistant Module-Lattice-Based Key Encapsulation Mechanism
  • 6d3becb: 8344861: Disable CheckJNICalls in tests until JDK-8344802 is fixed
  • ... and 155 more: https://git.openjdk.org/jdk/compare/c388455d0a463c9cb52ad18050f1155ec4ac0e6c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 22, 2024
@openjdk openjdk bot closed this Nov 22, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 22, 2024
@openjdk
Copy link

openjdk bot commented Nov 22, 2024

@lmesnik Pushed as commit 98b6678.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants