Skip to content
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

8274794: Print all owned locks in hs_err file #5958

Closed
wants to merge 4 commits into from

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Oct 14, 2021

See CR for details. This saves the created mutex in a mutex_array during Mutex construction so that all Mutex, not just the ones in mutexLocker.cpp can be printed in the hs_err file. ThreadCritical is used to protect the mutex_array, and to avoid UB should be used when printing the owned locks, but since that is done during error handling, this seemed better not to lock during error handling. There's 0 probability that another thread will be creating a lock during this error handling anyway.
Tested with tier1-8.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5958/head:pull/5958
$ git checkout pull/5958

Update a local copy of the PR:
$ git checkout pull/5958
$ git pull https://git.openjdk.java.net/jdk pull/5958/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5958

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5958.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 14, 2021

👋 Welcome back coleenp! 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 openjdk bot added the rfr label Oct 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 14, 2021

@coleenp The following label will be automatically applied to this pull request:

  • hotspot

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

@openjdk openjdk bot added the hotspot label Oct 14, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 14, 2021

Webrevs

Copy link
Member

@tstuefe tstuefe left a comment

Hi Coleen,

Should we limit this to debug-only, to not pay the costs for analysis we don't need?

Would it make sense to use a linked list instead? Removal would be far cheaper. Also, since every Mutex lives in this list anyway without exception, we could make the list pointers just part of the Mutex class itself, to save having to allocate a node.

I wonder whether we should, for global Mutexes, skip the destruction sequence altogether. Oh, I see the global mutexes are all new'ed, so we never delete them.

Printing on error is a bit of a problem with no perfect solution. If you print without locking, you risk crashes. But those would be caught by secondary error handling. But you don't get your full list. If you draw the lock OTOH, you may deadlock if the lock is already active, but that would be caught by the STEP timeout system too. So I think what you do is okay either way.

Cheers, Thomas

src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Coleen,

This looks good to me. One minor nit with the new test. :)

Thanks,
David

src/hotspot/share/runtime/mutex.cpp Show resolved Hide resolved
src/hotspot/share/runtime/mutexLocker.hpp Show resolved Hide resolved
@openjdk
Copy link

@openjdk openjdk bot commented Oct 15, 2021

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

8274794: Print all owned locks in hs_err file

Reviewed-by: stuefe, dholmes

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

  • a03119c: 8275436: [BACKOUT] JDK-8271949 dumppath in -XX:FlightRecorderOptions does not affect
  • bad75e6: 8275150: URLClassLoaderTable should store OopHandle instead of Handle
  • 72a976e: 8266936: Add a finalization JFR event
  • bcbe384: 8269175: [macosx-aarch64] wrong CPU speed in hs_err file
  • 426bcee: 8275360: Use @OverRide in javax.annotation.processing
  • 4d383b9: 8275298: Remove unnecessary weak_oops_do call in adjust weak roots phase
  • fb8e5cf: 8275368: Correct statement of kinds of elements Processor.process operates over
  • d548f2f: 8274346: Support for additional content in an app-image.
  • a619f89: 8274721: UnixSystem fails to provide uid, gid or groups if no username is available
  • 1afddb2: 8275322: Change nested classes in java.management to static nested classes
  • ... and 48 more: https://git.openjdk.java.net/jdk/compare/89999f70e06b41704c7c5b0f9a19582f90806a10...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 ready label Oct 15, 2021
@tstuefe
Copy link
Member

@tstuefe tstuefe commented Oct 15, 2021

Thinking this through further, I think that this is may not be the best approach.

The number of dynamically generated Mutexes can be very high, and their lifetime is unpredictable. E.g. because each ClassLoaderData is guarded by a Mutex. And we can have a lot of them (not sure if JEP 416 will do away with reflection-specific loaders, but just think of dynamic languages like jruby. Or just people who like class loaders :-).

I just ran a test with 5000 loaders and the array gets quite big, linear search is not good here. Especially since GrowableArray compacts itself on every removal.

My alternative proposal would be, as I wrote, to use a double-linked list. Removal would be O(1). And if you embed the list pointers in the Mutex itself, you don't need much more memory. Compared to your patch you'd pay 8 bytes per Mutex more because you need two pointers, but you'd save the overhead GrowableArray causes. Less allocation churn too since you save allocation of the array. And it also solves the problem of releasing memory if you release a bunch of Mutexes. With a GrowableArray, it has to shrink itself, but if you embed the list pointers in Mutex you get memory management for free.

Cheers, Thomas

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Oct 15, 2021

Thanks for looking at this Thomas and for the experiments. Mine weren't so large. I'll change it to a doubly linked list. That does seem safer and better.
I made Mutex smaller by removing the embedded char[] so I think we can handle another two pointers in the class. Although not ideal.

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Oct 15, 2021

The new printing in the error file looks like this. I'm not sure if it'll be confusing to see how many mutexes have been created, or maybe the message should be reworded somewhat?

VM Mutex/Monitor currently owned by a thread:
[0x0000146688024e00] Threads_lock - owner thread: 0x00001466880283c0 allow_vm_block safepoint-1
Total Mutex count 580

The allow_vm_block and safepoint-1 are NOT printed in PRODUCT mode.

I took out this because I don't know what it's trying to say and it doesn't seem meaningful:
// print format used by Mutex::print_on_error()
st->print_cr(" ([mutex/lock_event])");

Copy link
Member

@tstuefe tstuefe left a comment

Hi Coleen,

thank you for taking my suggestion.

I still think that this may be better suited for debug only, like the other analysis features of Mutexes, but I leave that up to you. There is a case for making this available in release builds too.

Apart from that, the rest are mostly unimportant nits. I'll leave it up to you what you take from them.

Cheers , Thomas

src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/utilities/vmError.cpp Show resolved Hide resolved
Copy link
Contributor Author

@coleenp coleenp left a comment

Thanks for the comments Thomas.

src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/utilities/vmError.cpp Show resolved Hide resolved
Copy link
Member

@tstuefe tstuefe left a comment

Hi Coleen, this looks good to me now. My remaining remarks are bikeshedding. Thanks for taking my suggestions.

..Thomas

src/hotspot/share/runtime/mutex.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/mutex.cpp Show resolved Hide resolved
@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Oct 18, 2021

Thomas, thank you for reviewing and all the good comments.

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Oct 21, 2021

Thank you for the review, David and Thomas.
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

Going to push as commit 819d2df.
Since your change was applied there have been 125 commits pushed to the master branch:

  • c41ce6d: 8275415: Prepare Leak Profiler for Lilliput
  • 0c3eaea: 8168388: GetMousePositionTest fails with the message "Mouse position should not be null"
  • 09f5235: 8275405: Linking error for classes with lambda template parameters and virtual functions
  • a120937: 8274988: G1: refine G1SegmentedArrayAllocOptions and G1CardSetAllocOptions
  • c7a80e6: 8275607: G1: G1CardSetAllocator::drop_all needs to call G1SegmentedArray::drop_all
  • af7c56b: 8275167: x86 intrinsic for unsignedMultiplyHigh
  • cea3f01: 8275666: serviceability/jvmti/GetObjectSizeClass.java shouldn't have vm.flagless
  • d1e3ca4: 8233558: [TESTBUG] WindowOwnedByEmbeddedFrameTest.java fails on macos
  • 913f928: 8273507: Convert test/jdk/java/nio/channels/Channels/TransferTo.java to TestNG test
  • 0021a2f: 8275449: Add linux-aarch64-zero build profile
  • ... and 115 more: https://git.openjdk.java.net/jdk/compare/89999f70e06b41704c7c5b0f9a19582f90806a10...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 21, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

@coleenp Pushed as commit 819d2df.

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

@coleenp coleenp deleted the mutex-array branch Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated
3 participants