8320652: ThreadInfo.isInNative needs to be updated to say what executing native code means #16791

AlanBateman · 2023-11-23T10:25:31Z

This is a docs only change to j.l.management.ThreadInfo::isInNative.

The method currently specifies that it tests if the thread is "executing native code via the Java Native Interface (JNI)". It would be clearer to say that it tests if the thread is executing a native method, and expand it to include native code invoked using the new foreign linker APIs. For now, I've left out the detail of a downcall handle created with the "critical" linker option.

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
Change requires CSR request JDK-8320658 to be approved

Issues

JDK-8320652: ThreadInfo.isInNative needs to be updated to say what executing native code means (Bug - P4)
JDK-8320658: ThreadInfo.isInNative needs to be updated to say what executing native code means (CSR)

Reviewers

Mandy Chung (@mlchung - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16791

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

Using diff file

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

Webrev

Link to Webrev Comment

AlanBateman · 2023-11-23T10:25:37Z

/csr

bridgekeeper · 2023-11-23T10:26:52Z

👋 Welcome back alanb! 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 · 2023-11-23T10:27:30Z

@AlanBateman has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@AlanBateman please create a CSR request for issue JDK-8320652 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2023-11-23T10:27:53Z

@AlanBateman 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 · 2023-11-24T14:24:13Z

Webrevs

jaikiran · 2023-11-25T07:32:20Z

src/java.management/share/classes/java/lang/management/ThreadInfo.java

+     * native code. This method returns {@code true} if the thread is executing
+     * a native method or if executing native code invoked using a {@linkplain
+     * java.lang.invoke.MethodHandle method handle} obtained from the
+     * {@linkplain java.lang.foreign.Linker native linker}.
     *


This area is new to me, but I happened to be in this code few days back. I'm mostly curious on what the actual definition of a thread being in native means.
When a thread is executing any of the following, does it end up being considered as being in a "native method":

A syscall (for example, write())

A C function exposed by a platform specific library

A JNI method (either part of the JDK or the application) which then may or may not do any syscall or C function call on a platform specific library

I would agree, it should state if runtime functions (including those doing a syscall) will be counted here. (For JNi i would not need it to be spelled out, on the other hand it would help, since it makes clear we don’t mean c2 code)

This isInNative method intends to provide a way to tell if the thread has transitioned from executing Java method to a native method. JVM interpreter and compiler provide the support for Java language which is why I think such clarification might not be highly necessary.

FWIW I would not remove the "does not include ..." information otherwise people may wrongly infer that in the past this method would not return true for VM code or generated code, but that now it does.

Note that there is no answer for the "syscall" case mentioned as it depends on the code that makes the syscall: if from a native method then yes; if from the VM then no.

Well.. it has to do in so far, that those threads are not hidden - the ThreadInfo sees it and says "isNative". But ok if this is not expected, then indeed it does not need to be documented.

seems to be no java method marked "native" on the stack, what case is that?

A native thread that is attached to the VM but is not currently executing any Java code.

funfact: its not in the list

JDWP filters its own internal threads out of any thread lists it returns, so this may be a case of using two different API's with different views of the set of threads running.

JDWP filters its own internal threads out of any thread lists it returns, so this may be a case of using two different API's with different views of the set of threads running.

I think the issue that Bernd is running into is a long standing anomaly between ThreadMXBean.getAllThreadIds() and the ThreadMXBean.getThreadInfo methods. The former uses a ThreadsListEnumerator with include_jvmti_agent_threads=false so it filters out the JVMTI agent threads. The latter doesn't do the filtering so if you have threadId of a JVMTI agent thread then you can get its thread info.

I've created JDK-8321095 to track the inconsistency that Bernd brought up on whether JVMTI agent threads are hidden or not by ThreadMXBean. It seems this inconsistency goes back a long way, to JDK 8 at least.

funfact: its not in the list

JDWP filters its own internal threads out of any thread lists it returns, so this may be a case of using two different API's with different views of the set of threads running.

JDWP does filter out its own threads (but not other JVMTI agent threads), but are we talking about JDWP here? It looks like Thread.print dcmd is producing the thread list that includes the JDWP threads, and as pointed out, ThreadMXBean.getAllThreadIds() does not include the JDWP threads (this is due to ThreadMXBean doing the filtering of JVMTI agent threads, not JDWP).

Note that threadId is clearly specified by ThreadMXBean, but there is no specification for the Thread.print output, so using unlabeled values from Thread.print as an argument to ThreadMXBean.getThreadInfo() seems somewhat presumptuous.

mlchung · 2023-11-27T19:19:18Z

It's ok to drop the sentence about "native code" does not include runtime or compiled code.

I can see why you left out the detail of a downcall handle. Maybe better to link the text "method handle" to Linker::downcallHandle that is how such a method handle is created.

AlanBateman · 2023-11-27T20:18:57Z

I can see why you left out the detail of a downcall handle. Maybe better to link the text "method handle" to Linker::downcallHandle that is how such a method handle is created.

That's a good idea, I've changed it to link to downcallHandle instead.

jaikiran · 2023-12-01T12:47:32Z

Thank you Alan for this latest update in commit 4836a9d - it's much more clear on what's considered native code.

jaikiran · 2023-12-01T12:49:05Z

src/java.management/share/classes/java/lang/management/ThreadInfo.java

+     * native code.
+     *
+     * <p> A thread is considered to be executing native code when it is executing a
+     * native method, executing native code invoked using a {@linkplain


Nit - should "native method" be "{@code native} method"?

mlchung

The updated spec change looks good.

openjdk · 2023-12-02T22:47:07Z

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

8320652: ThreadInfo.isInNative needs to be updated to say what executing native code means

Reviewed-by: mchung

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

9498469: 8318983: Fix comment typo in PKCS12Passwd.java
4dcbd13: 8314905: jdk/jfr/tool/TestView.java fails with RuntimeException 'Invoked Concurrent' missing from stdout/stderr
5dee2a3: 8320440: Implementation of Structured Concurrency (Second Preview)
6f7bb79: 8320931: [REDO] dsymutil command leaves around temporary directories
8be3e39: 8320129: "top" command during jtreg failure handler does not display CPU usage on OSX
2f299e4: 8321182: SourceExample.SOURCE_14 comment should refer to 'switch expressions' instead of 'text blocks'
3a09a05: 8313722: JFR: Avoid unnecessary calls to Events.from(Recording)
42af8ce: 8308614: Enabling JVMTI ClassLoad event slows down vthread creation by factor 10
1839433: 8320941: Discuss receiver type handling
92f7e51: 8312098: Update man page for javadoc
... and 6 more: https://git.openjdk.org/jdk/compare/ecd335d8f42757d332f217e220e1a9db8c48c8d6...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.

AlanBateman · 2023-12-05T12:09:25Z

/integrate

openjdk · 2023-12-05T12:11:42Z

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

672f373: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed
30817b7: 8317809: Insertion of free code blobs into code cache can be very slow during class unloading
a56286f: 8321269: Require platforms to define DEFAULT_CACHE_LINE_SIZE
1cf7ef5: 8321273: Parallel: Remove unused UpdateOnlyClosure::_space_id
517b178: 8306914: Implement JEP 458: Launch Multi-File Source-Code Programs
aec3865: 8320697: RISC-V: Small refactoring for runtime calls
50d1839: 8318809: java/util/concurrent/ConcurrentLinkedQueue/WhiteBox.java shows intermittent failures on linux ppc64le and aarch64
81484d8: 8320687: sun.jvmstat.monitor.MonitoredHost.getMonitoredHost() throws unexpected exceptions when invoked concurrently
30b5d42: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
bd04f91: 8321131: Console read line with zero out should zero out underlying buffer in JLine
... and 34 more: https://git.openjdk.org/jdk/compare/ecd335d8f42757d332f217e220e1a9db8c48c8d6...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-05T12:11:50Z

@AlanBateman Pushed as commit 4fbf22b.

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

Initial commit

c60f176

openjdk bot added the csr Pull request needs approved CSR before integration label Nov 23, 2023

openjdk bot added the serviceability serviceability-dev@openjdk.org label Nov 23, 2023

AlanBateman added 2 commits November 24, 2023 10:31

Merge

4fa2ed3

Simplify wording

93ac45a

AlanBateman marked this pull request as ready for review November 24, 2023 14:18

openjdk bot added the rfr Pull request is ready for review label Nov 24, 2023

jaikiran reviewed Nov 25, 2023

View reviewed changes

AlanBateman added 2 commits November 27, 2023 19:56

Merge

d8930bb

Change link to downcallHandle

5e2cc18

mlchung approved these changes Nov 27, 2023

View reviewed changes

AlanBateman added 2 commits December 1, 2023 08:07

Merge

9f4bca5

Extend description

4836a9d

jaikiran reviewed Dec 1, 2023

View reviewed changes

mlchung approved these changes Dec 1, 2023

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Dec 2, 2023

openjdk bot added the integrated Pull request has been integrated label Dec 5, 2023

openjdk bot closed this Dec 5, 2023

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

8320652: ThreadInfo.isInNative needs to be updated to say what executing native code means #16791

8320652: ThreadInfo.isInNative needs to be updated to say what executing native code means #16791

Uh oh!

Conversation

AlanBateman commented Nov 23, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

AlanBateman commented Nov 23, 2023

Uh oh!

bridgekeeper bot commented Nov 23, 2023

Uh oh!

openjdk bot commented Nov 23, 2023

Uh oh!

openjdk bot commented Nov 23, 2023

Uh oh!

mlbridge bot commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlchung commented Nov 27, 2023

Uh oh!

AlanBateman commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaikiran commented Dec 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 2, 2023

Uh oh!

AlanBateman commented Dec 5, 2023

Uh oh!

openjdk bot commented Dec 5, 2023

Uh oh!

openjdk bot commented Dec 5, 2023

Uh oh!

Uh oh!

AlanBateman commented Nov 23, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 24, 2023 •

edited

Loading

AlanBateman Nov 30, 2023 •

edited

Loading

AlanBateman commented Nov 27, 2023 •

edited

Loading