8322980: Debug agent's dumpThread() API should update thread's name before printing it #17259

plummercj · 2024-01-04T02:06:28Z

In threadControl.c, at build time you can decide to keep track of thread names by compiling with "#define DEBUG_THREADNAME". If this is also a debug build, some extra debugging functions are included in the build, including "dumpThread(ThreadNode *node)". These are intended to be called from gdb, or possibly from somewhere in the debug agent implementation, to aid with debugging. When dumpThread() prints the thread's name, it just uses the name that was stored when the thread was created. However, the thread name can change, so dumpThread() should really fetch the current thread name and print it.

I also added some commented out code to print other useful fields of the ThreadNode, including the ThreadState. These can be enabled by the user as needed.

Tested with all of tier1, and also ran tier2 and tier4 svc tests.

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-8322980: Debug agent's dumpThread() API should update thread's name before printing it (Enhancement - P5)

Reviewers

Kevin Walls (@kevinjwalls - Committer) ⚠️ Review applies to eb318d81
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17259

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-04T02:07:56Z

👋 Welcome back cjplummer! 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-01-04T02:09:19Z

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

serviceability

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.

mlbridge · 2024-01-04T02:14:48Z

Webrevs

01: Full - Incremental (fbbd07e5)
00: Full (eb318d81)

kevinjwalls

Looks good I think.

A future idea could be to have these ifdefs active in the debug JDK build. I wouldn't normally run the debugger from the debug build, but if you wanted this extra info it would be more convenient if it was in the debug JDK bundle.

(The changed output might fool some tests so would need checking, but might be worth considering.)

plummercj · 2024-01-04T17:21:14Z

A future idea could be to have these ifdefs active in the debug JDK build. I wouldn't normally run the debugger from the debug build, but if you wanted this extra info it would be more convenient if it was in the debug JDK bundle.

Possibly DEBUG_THREADNAME should be enabled in debug builds, but I opted not to long ago because of the potential footprint and performance cost. For the #if 0 code, generally speaking it results in too much output and is really just meant as examples of other things you might want to include in the output, but most cases you would prefer not to have all of it. A possible solution would be to have each line of output controlled by a global variable (either a bitwise flag or a global per line), and then the user could enable additional output at runtime (from within gdb), but I don't feel this is worth the effort.

There aren't any test output concern unless you introduce calls to these API, in which case they are a concern whether or not all the #if 0 stuff is included.

plummercj · 2024-01-05T17:52:32Z

Could I get one more review please.

sspitsyn · 2024-01-05T18:37:00Z

src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c

+  jint state = 0;
+  jvmtiError error = FUNC_PTR(gdata->jvmti,GetThreadState)
+    (gdata->jvmti, node->thread, &state);
+  return state;


Nit: The indent in this file is 4, not 2.

sspitsyn

Looks good.
Q: Do you want to keep the debugging code under #ifdef ... #endif?

openjdk · 2024-01-05T18:40:50Z

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

8322980: Debug agent's dumpThread() API should update thread's name before printing it

Reviewed-by: kevinw, sspitsyn

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

35a1b77: 8322636: [JVMCI] HotSpotSpeculationLog can be inconsistent across a single compile
46965a0: 8322981: Fix 2 locations in JDI that throw IOException without using the "Caused by" exception
700c25f: 8322954: Shenandoah: Convert evac-update closures asserts to rich asserts
631a9f6: 8323073: ProblemList gc/g1/TestSkipRebuildRemsetPhase.java on linux-aarch64
ed9f324: 8322985: [BACKOUT] 8318562: Computational test more than 2x slower when AVX instructions are used
ade21a9: 8310844: [AArch64] C1 compilation fails because monitor offset in OSR buffer is too large for immediate
f0cfd36: 8322532: JShell : Unnamed variable issue
78623c9: 8323012: C2 fails with fatal error: no reachable node should have no use
f0e2e43: 8323021: Shenandoah: Encountered reference count always attributed to first worker thread
3dc4bd8: 8322989: New test serviceability/HeapDump/FullGCHeapDumpLimitTest.java fails
... and 24 more: https://git.openjdk.org/jdk/compare/54b3ceeca27b67f4270d8b700b072f46959dba65...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.

plummercj · 2024-01-05T19:02:38Z

Looks good. Q: Do you want to keep the debugging code under #ifdef ... #endif?

Yes. The "#ifdef DEBUG_THREADNAME" is something that I want the user to have to manually enable at build time to avoid the memory and performance impact. Also, "#if 0" code generates extra output that normally you don't want, but I wanted to leave the code there as examples and so it can be easily enabled when needed."

plummercj · 2024-01-08T16:54:33Z

Thank you for the reviews Kevin and Serguei!

/integrate

openjdk · 2024-01-08T16:56:45Z

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

c4a83bd: 8323086: Shenandoah: Heap could be corrupted by oom during evacuation
827c71d: 8310190: C2 SuperWord: AlignVector is broken, generates misaligned packs
2acb5bd: 8322790: RISC-V: Tune costs for shuffles with no conversion
c8fa3e2: 8320310: CompiledMethod::has_monitors flag can be incorrect
57a65fe: 8322003: JShell - Incorrect type inference in lists of records implementing interfaces
c90768c: 8318444: Write details about compilation bailouts into crash reports
29397d2: 8320317: ObjectMonitor NotRunnable is not really an optimization
fc04750: 8321371: SpinPause() not implemented for bsd_aarch64/macOS
458e563: 8310711: [IR Framework] Remove safepoint while printing handling
71aac7a: 8276809: java/awt/font/JNICheck/FreeTypeScalerJNICheck.java shows JNI warning on Windows
... and 44 more: https://git.openjdk.org/jdk/compare/54b3ceeca27b67f4270d8b700b072f46959dba65...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-01-08T16:56:51Z

@plummercj Pushed as commit 387828a.

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

plummercj added 3 commits January 4, 2024 02:01

Set get/set the thread's current name when dumping threads

b36d82e

Undo change to not print out suspendCount by default.

a4b1c10

Minor code cleanup.

eb318d8

openjdk bot changed the title ~~8322980~~ 8322980: Debug agent's dumpThread() API should update thread's name before printing it Jan 4, 2024

openjdk bot added the serviceability serviceability-dev@openjdk.org label Jan 4, 2024

plummercj marked this pull request as ready for review January 4, 2024 02:10

openjdk bot added the rfr Pull request is ready for review label Jan 4, 2024

kevinjwalls approved these changes Jan 4, 2024

View reviewed changes

sspitsyn reviewed Jan 5, 2024

View reviewed changes

sspitsyn approved these changes Jan 5, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 5, 2024

fix indentation

fbbd07e

sspitsyn approved these changes Jan 5, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 8, 2024

openjdk bot closed this Jan 8, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8322980: Debug agent's dumpThread() API should update thread's name before printing it #17259

8322980: Debug agent's dumpThread() API should update thread's name before printing it #17259

plummercj commented Jan 4, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 4, 2024

openjdk bot commented Jan 4, 2024

mlbridge bot commented Jan 4, 2024 •

edited

Loading

kevinjwalls left a comment

plummercj commented Jan 4, 2024

plummercj commented Jan 5, 2024

sspitsyn Jan 5, 2024

plummercj Jan 5, 2024

sspitsyn left a comment

openjdk bot commented Jan 5, 2024 •

edited

Loading

plummercj commented Jan 5, 2024

plummercj commented Jan 8, 2024

openjdk bot commented Jan 8, 2024

openjdk bot commented Jan 8, 2024

8322980: Debug agent's dumpThread() API should update thread's name before printing it #17259

8322980: Debug agent's dumpThread() API should update thread's name before printing it #17259

Conversation

plummercj commented Jan 4, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jan 4, 2024

openjdk bot commented Jan 4, 2024

mlbridge bot commented Jan 4, 2024 • edited Loading

Webrevs

kevinjwalls left a comment

Choose a reason for hiding this comment

plummercj commented Jan 4, 2024

plummercj commented Jan 5, 2024

sspitsyn Jan 5, 2024

Choose a reason for hiding this comment

plummercj Jan 5, 2024

Choose a reason for hiding this comment

sspitsyn left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 5, 2024 • edited Loading

plummercj commented Jan 5, 2024

plummercj commented Jan 8, 2024

openjdk bot commented Jan 8, 2024

openjdk bot commented Jan 8, 2024

plummercj commented Jan 4, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 4, 2024 •

edited

Loading

openjdk bot commented Jan 5, 2024 •

edited

Loading