8329109: Threads::print_on() tries to print CPU time for terminated GC threads #18518

reinrich · 2024-03-27T16:09:46Z

EDIT 2024-04-02: Use Threads::non_java_threads_do(ThreadClosure* tc) to print non-java threads (commit b2dabc9). It uses thread list iterators that synchronize with thread creation and termination. If a non-java thread is about to terminate, it waits for in progress iterations (see NonJavaThread::remove_from_the_list).

Testing:

The output of a minimal example with G1 is almost identical (attached below). The order of non-java threads can differ.

The fix passed our CI testing: JTReg tests: tier1-4 of hotspot and jdk. All of Langtools and jaxp. JCK, SPECjvm2008, SPECjbb2015, Renaissance Suite, and SAP specific tests.
Testing was done with fastdebug builds on the main platforms and also on Linux/PPC64le.

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-8329109: Threads::print_on() tries to print CPU time for terminated GC threads (Bug - P3)

Reviewers

Matthias Baesken (@MBaesken - Reviewer)
David Holmes (@dholmes-ora - Reviewer)
Albert Mingkun Yang (@albertnetymk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18518

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-27T16:10:42Z

👋 Welcome back rrich! 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 · 2024-03-27T16:11:17Z

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

8329109: Threads::print_on() tries to print CPU time for terminated GC threads

Reviewed-by: mbaesken, dholmes, ayang

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

233619b: 8329557: Fix statement around MathContext.DECIMAL128 rounding
023f7f1: 8320799: Bump minimum boot jdk to JDK 22
8dc43aa: 8325217: MethodSymbol.getModifiers() returns SEALED for restricted methods
1c69193: 8328383: Method is not used: com.sun.tools.javac.comp.Attr::thisSym
ee09801: 8328352: Serial: Inline SerialBlockOffsetSharedArray
bea493b: 8236736: Change notproduct JVM flags to develop flags
80c54b4: 8328932: Parallel: Proper partial object setup in fill_dense_prefix_end
d954f3b: 8329493: Parallel: Remove unused ParallelArguments::heap_max_size_bytes
bdd9438: 8328647: TestGarbageCollectorMXBean.java fails with C1-only and -Xcomp
e3e6c2a: 8328278: Do not print the tenuring threshold in AgeTable::print_on
... and 70 more: https://git.openjdk.org/jdk/compare/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19...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 · 2024-03-27T16:11:36Z

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

hotspot-runtime

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.

reinrich · 2024-03-28T07:27:40Z

/label add hotspot-gc

openjdk · 2024-03-28T07:28:05Z

@reinrich
The hotspot-gc label was successfully added.

mlbridge · 2024-03-28T07:40:43Z

Webrevs

01: Full - Incremental (b2dabc95)
00: Full (568a2bcc)

albertnetymk · 2024-03-28T13:14:49Z

It's Universe::heap()->gc_threads_do that may contain terminated threads, not any other kinds ofNonJavaThreads, right?

If so, can the fix be placed in gc_threads_do, where every collector ensures only active gc threads are visited.

(I see that David made a similar remark in the bug report.)

reinrich · 2024-03-28T15:14:21Z

It's Universe::heap()->gc_threads_do that may contain terminated threads, not any other kinds ofNonJavaThreads, right?

WatcherThread, NativeHeapTrimmerThread can also be terminated. The latter is not shown by Threads::print_on though (even Threads::print_on_error doesn't show it which surprises me a little bit).

WatcherThread::watcher_thread() will return null when the WatcherThread was stopped. So the issue is rather unlikely.

If so, can the fix be placed in gc_threads_do, where every collector ensures only active gc threads are visited.

(I see that David made a similar remark in the bug report.)

Ok. I'll wait a bit and shall close this pr if nobody thinks this approach should be taken.

(Btw: G1CollectedHeap::stop() doesn't stop G1CollectedHeap::_cm. Likely an oversight)

albertnetymk · 2024-03-28T16:27:34Z

WatcherThread, NativeHeapTrimmerThread can also be terminated.

I see.

I notice that Thread::print_on contains if (osthread() != nullptr) { to skip some threads. Is it possible to expand this condition to skip zombie ones? (Or set _osthread to nullptr when a thread enters zombie state.)

(Btw: G1CollectedHeap::stop() doesn't stop G1CollectedHeap::_cm. Likely an oversight)

_cm is not a thread; it's the data structure used by _cm_thread. Therefore, it shouldn't/can't be stopped.

reinrich · 2024-03-28T16:43:44Z

WatcherThread, NativeHeapTrimmerThread can also be terminated.

I see.

I notice that Thread::print_on contains if (osthread() != nullptr) { to skip some threads. Is it possible to expand this condition to skip zombie ones? (Or set _osthread to nullptr when a thread enters zombie state.)

A very simple alternative could be to check thread->osthread()->get_state() in fast_cpu_time and just return -1 if the thread isn't INITIALIZED yet or already ZOMBIE.

(Btw: G1CollectedHeap::stop() doesn't stop G1CollectedHeap::_cm. Likely an oversight)

_cm is not a thread; it's the data structure used by _cm_thread. Therefore, it shouldn't/can't be stopped.

It has _concurrent_workers but even though they seem to work concurrently to the mutator they are just WorkerThread and not ConcurrentGCThread. Only the latter type of thread can be stopped.

This reverts commit 568a2bc.

reinrich · 2024-04-02T13:38:32Z

Thread dump of minimal example with G1.
Baseline: thread_dump_baseline.txt
PR: thread_dump_pr.txt

MBaesken

Looks like a good and simple fix, thanks ! The doc of NonJavaThread::Iterator says 'only live fully-initialized threads can be found in the list' and this is what we need here.

dholmes-ora · 2024-04-03T12:14:06Z

src/hotspot/share/runtime/threads.cpp

-  Universe::heap()->gc_threads_do(&cl);
-  cl.do_thread(WatcherThread::watcher_thread());
-  cl.do_thread(AsyncLogWriter::instance());
+  non_java_threads_do(&cl);


This certainly looks neater and simpler and ensures all NJT's will be processed. There is additional synchronization involved in using the NonJavaThreads::Iterator that I had to look closely at but I think it is safe to use in this context.

reinrich · 2024-04-05T06:25:48Z

Thanks for the reviews. I think this is ready now...
/integrate

openjdk · 2024-04-05T06:27:09Z

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

5860a48: 8329624: Add visitors for preview language features
0b01144: 8329720: Gtest failure printing markword after JDK-8325303
34f7974: 8325303: Replace markWord.is_neutral() with markWord.is_unlocked()
27cfcef: 8329651: TestLibGraal.java crashes with assert(_stack_base != nullptr)
e1183ac: 8329703: Remove unused apple.jpeg file from SwingSet2 demo
12ad09a: 8322042: HeapDumper should perform merge on the current thread instead of VMThread
d80d478: 8328649: Disallow enclosing instances for local classes in constructor prologues
83eba86: 8329332: Remove CompiledMethod and CodeBlobLayout classes
28216aa: 8328366: Thread.setContextClassloader from thread in FJP commonPool task no longer works after JDK-8327501
4276d5c: 8329637: Apparent typo in java.security file property jdk.tls.keyLimits
... and 96 more: https://git.openjdk.org/jdk/compare/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-05T06:27:14Z

@reinrich Pushed as commit c1cfb43.

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

mmyxym · 2024-05-17T09:04:03Z

/backport jdk21u-dev

openjdk · 2024-05-17T09:04:45Z

@mmyxym the backport was successfully created on the branch backport-mmyxym-c1cfb43d in my personal fork of openjdk/jdk21u-dev. To create a pull request with this backport targeting openjdk/jdk21u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit c1cfb43d from the openjdk/jdk repository.

The commit being backported was authored by Richard Reingruber on 5 Apr 2024 and was reviewed by Matthias Baesken, David Holmes and Albert Mingkun Yang.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u-dev:

$ git fetch https://github.com/openjdk-bots/jdk21u-dev.git backport-mmyxym-c1cfb43d:backport-mmyxym-c1cfb43d
$ git checkout backport-mmyxym-c1cfb43d
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u-dev.git backport-mmyxym-c1cfb43d

mmyxym · 2024-05-20T02:39:55Z

/backport jdk22u

openjdk · 2024-05-20T02:40:42Z

@mmyxym the backport was successfully created on the branch backport-mmyxym-c1cfb43d in my personal fork of openjdk/jdk22u. To create a pull request with this backport targeting openjdk/jdk22u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit c1cfb43d from the openjdk/jdk repository.

The commit being backported was authored by Richard Reingruber on 5 Apr 2024 and was reviewed by Matthias Baesken, David Holmes and Albert Mingkun Yang.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk22u:

$ git fetch https://github.com/openjdk-bots/jdk22u.git backport-mmyxym-c1cfb43d:backport-mmyxym-c1cfb43d
$ git checkout backport-mmyxym-c1cfb43d
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22u.git backport-mmyxym-c1cfb43d

Fix

568a2bc

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Mar 27, 2024

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Mar 28, 2024

reinrich marked this pull request as ready for review March 28, 2024 07:36

openjdk bot added the rfr Pull request is ready for review label Mar 28, 2024

reinrich added 2 commits April 2, 2024 11:02

Revert

93e79dd

This reverts commit 568a2bc.

Equivalent but simpler fix

b2dabc9

MBaesken approved these changes Apr 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 3, 2024

dholmes-ora approved these changes Apr 3, 2024

View reviewed changes

albertnetymk approved these changes Apr 3, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 5, 2024

openjdk bot closed this Apr 5, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8329109: Threads::print_on() tries to print CPU time for terminated GC threads #18518

8329109: Threads::print_on() tries to print CPU time for terminated GC threads #18518

reinrich commented Mar 27, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 27, 2024

openjdk bot commented Mar 27, 2024 •

edited

Loading

openjdk bot commented Mar 27, 2024

reinrich commented Mar 28, 2024

openjdk bot commented Mar 28, 2024

mlbridge bot commented Mar 28, 2024 •

edited

Loading

albertnetymk commented Mar 28, 2024

reinrich commented Mar 28, 2024

albertnetymk commented Mar 28, 2024

reinrich commented Mar 28, 2024

reinrich commented Apr 2, 2024

MBaesken left a comment

dholmes-ora Apr 3, 2024

reinrich commented Apr 5, 2024

openjdk bot commented Apr 5, 2024

openjdk bot commented Apr 5, 2024

mmyxym commented May 17, 2024

openjdk bot commented May 17, 2024

mmyxym commented May 20, 2024

openjdk bot commented May 20, 2024

8329109: Threads::print_on() tries to print CPU time for terminated GC threads #18518

8329109: Threads::print_on() tries to print CPU time for terminated GC threads #18518

Conversation

reinrich commented Mar 27, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 27, 2024

openjdk bot commented Mar 27, 2024 • edited Loading

openjdk bot commented Mar 27, 2024

reinrich commented Mar 28, 2024

openjdk bot commented Mar 28, 2024

mlbridge bot commented Mar 28, 2024 • edited Loading

Webrevs

albertnetymk commented Mar 28, 2024

reinrich commented Mar 28, 2024

albertnetymk commented Mar 28, 2024

reinrich commented Mar 28, 2024

reinrich commented Apr 2, 2024

MBaesken left a comment

Choose a reason for hiding this comment

dholmes-ora Apr 3, 2024

Choose a reason for hiding this comment

reinrich commented Apr 5, 2024

openjdk bot commented Apr 5, 2024

openjdk bot commented Apr 5, 2024

mmyxym commented May 17, 2024

openjdk bot commented May 17, 2024

mmyxym commented May 20, 2024

openjdk bot commented May 20, 2024

reinrich commented Mar 27, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 27, 2024 •

edited

Loading

mlbridge bot commented Mar 28, 2024 •

edited

Loading