Re: RFR: 8312174: missing JVMTI events from vthreads parked during JVMTI attach [v2]
> This update fixes two important issues: > - Issue reported by a bug submitter about missing JVMTI events on virtual > threads after an a JVMTI agent dynamic attach > - Known scalability/performance issue: a need to lazily create > `JvmtiThreadState's` for virtual threads > > The issue is tricky to fix because the existing mechanism of the JVMTI event > management does not support unmounted virtual threads. The JVMTI > `SetEventNotificationMode()` calls the function > `JvmtiEventControllerPrivate::recompute_enabled()` > which inspects a `JavaThread's` list and for each thread in the list > recomputes enabled event bits with the function > `JvmtiEventControllerPrivate::recompute_thread_enabled()`. The > `JvmtiThreadState` of each thread is created but only when it is really > needed, eg, if any of the thread filtered events is enabled. There was an > initial adjustment of this mechanism for virtual threads which accounted for > both carrier and virtual threads when a virtual thread is mounted. However, > it does not work for unmounted virtual threads. A temporary work around was > to always create `JvmtiThreadState` for each virtual thread eagerly at a > thread starting point. > > This fix introduces new function `JvmtiExport::get_jvmti_thread_state()` > which checks if thread is virtual and there is a thread filtered event > enabled globally, and if so, forces a creation of the `JvmtiThreadState`. > Another adjustment was needed because the function `state_for_while_locked()` > can be called directly in some contexts. New function > `JvmtiEventController::recompute_thread_filtered()` was introduced to make > necessary corrections. > > Testing: > - new test from the bug report was adopted: > `test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest` > - ran mach5 tiers 1-6: all are passed Serguei Spitsyn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge - 8312174: missing JVMTI events from vthreads parked during JVMTI attach - Changes: - all: https://git.openjdk.org/jdk/pull/15467/files - new: https://git.openjdk.org/jdk/pull/15467/files/dbe3a64a..dd97dacc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15467&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15467&range=00-01 Stats: 16961 lines in 424 files changed: 12615 ins; 2655 del; 1691 mod Patch: https://git.openjdk.org/jdk/pull/15467.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15467/head:pull/15467 PR: https://git.openjdk.org/jdk/pull/15467
Re: Question on why sun.management MBeans are not exported?
Hi, It would be a shame to lose these metrics because many of them have been very useful over time and some would be even more useful with some modifications. For example, the CPU breakouts found in GC logs has been incredibly useful as a proxy measure in helping sort out other issues in systems. So much so that I have analytics built specifically around this in my tooling. Kind regards, Kirk Pepperdine > On Sep 6, 2023, at 10:50 AM, Alan Bateman wrote: > > On 06/09/2023 16:17, Volker Simonis wrote: >> : >> I'm familiar with JEP 260. But wouldn't you agree that an >> "encapsulated" monitoring API is an oxymoron? A monitoring API is by >> design intended for external usage and completely useless to the >> platform itself. There's no single usage of the "sun.management" >> MBeans in the JDK itself (except for jconsole where the encapsulation >> broke it). My assumption is that the corresponding MBeans in >> "sun.management" are there for historic reasons (added in JDK 1.5) and >> would have made much more sense in "com.sun.management" package. But I >> doubt that they can be classified in the "internal implementation >> details of the JDK and never intended for external use² category of >> JEP 260. > It's left over from experiments on exposing some internal metrics, I think > during JDK 5. It's code that should probably have been removed a long time > ago. > > -Alan
Re: RFR: 8314021: HeapDump: Optimize segmented heap file merging phase
On Thu, 7 Sep 2023 00:56:40 GMT, Alex Menkov wrote: > The fix looks good to me in general, but I'm not sure about code > organization. src/hotspot/share/runtime/os.hpp describes rules for os* files. > It states: // Platform-independent source files should not include these > header files // (although sadly there are some rare exceptions ...) And the > change adds one more exception. I'd like to hear runtime guys opinion. Thanks for the review, yes, grepping `os_linux.hpp` found several exceptions, I'd like to see if runtime forks have alternatives, too. - PR Comment: https://git.openjdk.org/jdk/pull/15245#issuecomment-1709371785
Re: RFR: 8314021: HeapDump: Optimize segmented heap file merging phase [v2]
> This patch reduce ~16%(24s->20s) pahse 2 merge time during dumping 32g heap > with 96threads and fixes a memory leak of compressor > > You might argue why this is Linux-only optimization, because sendfile > requires at least socket fd in other platforms([aix > sendfile](https://www.ibm.com/docs/en/aix/7.1?topic=s-send-file-subroutine) > [maxos > sendfile](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/sendfile.2.html) > [win32 > TransmitFile](https://learn.microsoft.com/en-us/windows/win32/api/mswsock/nf-mswsock-transmitfile)), > while [only Linux](https://man7.org/linux/man-pages/man2/sendfile.2.html) > supports both two file descriptors. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: one merge_file for all - Changes: - all: https://git.openjdk.org/jdk/pull/15245/files - new: https://git.openjdk.org/jdk/pull/15245/files/ba381aaf..0eb02c14 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15245&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15245&range=00-01 Stats: 16 lines in 1 file changed: 4 ins; 9 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/15245.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15245/head:pull/15245 PR: https://git.openjdk.org/jdk/pull/15245
Re: RFR: JDK-8315486: vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn002/forceEarlyReturn002.java timed out [v2]
> To test ForceEarlyReturn command for NO_MORE_FRAMES case the test creates > ThreadStartEventRequest with SUSPEND_ALL policy and requests debuggee to > start new thread. > If debuggee JVM starts some internal threads before the request is cleared > (i.e. we have several ThreadStart events), 2nd event suspends debuggee again > and the test fails with timeout. > The change adds THREAD_ONLY modifier to the ThreadStartEventRequest to > generate event only for desired thread. > It requires thread ID, so debuggee was updated to create Thread object in > advance, debugger reads the thread ID from static field (it does not need to > be static, but Debugee class has convenient methods to retrieve class ID and > static field value). > > Testing: 100 runs of the test on > windows-x64-debug,linux-x64-debug,macosx-x64-debug with > JTREG_TEST_THREAD_FACTORY=Virtual, with and without "-XX:+UseZGC > -XX:+ZGenerational" Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: Chris feedback - enhanced comment - Changes: - all: https://git.openjdk.org/jdk/pull/15601/files - new: https://git.openjdk.org/jdk/pull/15601/files/310115de..5736e1c6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15601&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15601&range=00-01 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15601.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15601/head:pull/15601 PR: https://git.openjdk.org/jdk/pull/15601
Re: RFR: 8314021: HeapDump: Optimize segmented heap file merging phase
On Fri, 11 Aug 2023 09:31:56 GMT, Yi Yang wrote:
> This patch reduce ~16%(24s->20s) pahse 2 merge time during dumping 32g heap
> with 96threads and fixes a memory leak of compressor
>
> You might argue why this is Linux-only optimization, because sendfile
> requires at least socket fd in other platforms([aix
> sendfile](https://www.ibm.com/docs/en/aix/7.1?topic=s-send-file-subroutine)
> [maxos
> sendfile](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/sendfile.2.html)
> [win32
> TransmitFile](https://learn.microsoft.com/en-us/windows/win32/api/mswsock/nf-mswsock-transmitfile)),
> while [only Linux](https://man7.org/linux/man-pages/man2/sendfile.2.html)
> supports both two file descriptors.
The fix looks good to me in general, but I'm not sure about code organization.
src/hotspot/share/runtime/os.hpp describes rules for os* files.
It states:
// Platform-independent source files should not include these header files
// (although sadly there are some rare exceptions ...)
And the change adds one more exception.
I'd like to hear runtime guys opinion.
src/hotspot/share/services/heapDumper.cpp line 1651:
> 1649: // read+write combination, which would require transferring data
> to and from
> 1650: // user space.
> 1651: merge_file_fast(path);
Looks like you don't need separate method for linux.
Would it be better to make linux-specific implementation of merge_file():
#ifdef LINUX
// Merge segmented heap files via sendfile
void DumpMerger::merge_file(char* path) {
...
}
#else
// Generic implementation using read+write
void DumpMerger::merge_file(char* path) {
...
}
#endif
-
PR Comment: https://git.openjdk.org/jdk/pull/15245#issuecomment-1709311297
PR Review Comment: https://git.openjdk.org/jdk/pull/15245#discussion_r1317959545
Re: RFR: JDK-8315486: vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn002/forceEarlyReturn002.java timed out
On Wed, 6 Sep 2023 20:02:44 GMT, Alex Menkov wrote: > To test ForceEarlyReturn command for NO_MORE_FRAMES case the test creates > ThreadStartEventRequest with SUSPEND_ALL policy and requests debuggee to > start new thread. > If debuggee JVM starts some internal threads before the request is cleared > (i.e. we have several ThreadStart events), 2nd event suspends debuggee again > and the test fails with timeout. > The change adds THREAD_ONLY modifier to the ThreadStartEventRequest to > generate event only for desired thread. > It requires thread ID, so debuggee was updated to create Thread object in > advance, debugger reads the thread ID from static field (it does not need to > be static, but Debugee class has convenient methods to retrieve class ID and > static field value). > > Testing: 100 runs of the test on > windows-x64-debug,linux-x64-debug,macosx-x64-debug with > JTREG_TEST_THREAD_FACTORY=Virtual, with and without "-XX:+UseZGC > -XX:+ZGenerational" Overall looks good. Just one minor comment suggestion. test/hotspot/jtreg/vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn002/forceEarlyReturn002.java line 164: > 162: command.addByte(JDWP.EventKind.THREAD_START); > 163: command.addByte(JDWP.SuspendPolicy.ALL); > 164: // THREAD_ONLY modifier Maybe add a bit more of a comment here that explains we only want the THREAD_START event for the specified test thread and not any others that might start up. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15601#pullrequestreview-1614398877 PR Review Comment: https://git.openjdk.org/jdk/pull/15601#discussion_r1317959286
RFR: JDK-8315486: vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn002/forceEarlyReturn002.java timed out
To test ForceEarlyReturn command for NO_MORE_FRAMES case the test creates ThreadStartEventRequest with SUSPEND_ALL policy and requests debuggee to start new thread. If debuggee JVM starts some internal threads before the request is cleared (i.e. we have several ThreadStart events), 2nd event suspends debuggee again and the test fails with timeout. The change adds THREAD_ONLY modifier to the ThreadStartEventRequest to generate event only for desired thread. It requires thread ID, so debuggee was updated to create Thread object in advance, debugger reads the thread ID from static field (it does not need to be static, but Debugee class has convenient methods to retrieve class ID and static field value). Testing: 100 runs of the test on windows-x64-debug,linux-x64-debug,macosx-x64-debug with JTREG_TEST_THREAD_FACTORY=Virtual, with and without "-XX:+UseZGC -XX:+ZGenerational" - Commit messages: - jcheck - forceEarlyReturn002 Changes: https://git.openjdk.org/jdk/pull/15601/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15601&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8315486 Stats: 44 lines in 2 files changed: 34 ins; 7 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/15601.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15601/head:pull/15601 PR: https://git.openjdk.org/jdk/pull/15601
Re: RFR: 8312174: missing JVMTI events from vthreads parked during JVMTI attach
On Tue, 29 Aug 2023 10:09:21 GMT, Serguei Spitsyn wrote:
> This update fixes two important issues:
> - Issue reported by a bug submitter about missing JVMTI events on virtual
> threads after an a JVMTI agent dynamic attach
> - Known scalability/performance issue: a need to lazily create
> `JvmtiThreadState's` for virtual threads
>
> The issue is tricky to fix because the existing mechanism of the JVMTI event
> management does not support unmounted virtual threads. The JVMTI
> `SetEventNotificationMode()` calls the function
> `JvmtiEventControllerPrivate::recompute_enabled()`
> which inspects a `JavaThread's` list and for each thread in the list
> recomputes enabled event bits with the function
> `JvmtiEventControllerPrivate::recompute_thread_enabled()`. The
> `JvmtiThreadState` of each thread is created but only when it is really
> needed, eg, if any of the thread filtered events is enabled. There was an
> initial adjustment of this mechanism for virtual threads which accounted for
> both carrier and virtual threads when a virtual thread is mounted. However,
> it does not work for unmounted virtual threads. A temporary work around was
> to always create `JvmtiThreadState` for each virtual thread eagerly at a
> thread starting point.
>
> This fix introduces new function `JvmtiExport::get_jvmti_thread_state()`
> which checks if thread is virtual and there is a thread filtered event
> enabled globally, and if so, forces a creation of the `JvmtiThreadState`.
> Another adjustment was needed because the function `state_for_while_locked()`
> can be called directly in some contexts. New function
> `JvmtiEventController::recompute_thread_filtered()` was introduced to make
> necessary corrections.
>
> Testing:
> - new test from the bug report was adopted:
> `test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest`
> - ran mach5 tiers 1-6: all are passed
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
line 91:
> 89:
> 90: try (ExecutorService executorService =
> Executors.newVirtualThreadPerTaskExecutor()) {
> 91: for (int tCnt = 0; tCnt < TCNT1; tCnt++) {
Could you please add a comment before each test group creation block about
expected state
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
line 100:
> 98: mready.await();
> 99: try {
> 100: // timeout is big enough to keep mounted untill
> interrupted
The comment is misleading. 1st group of threads are expected to be unmounted
during attach and mounted after the threads are interrupted.
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
line 136:
> 134: ready1.await();
> 135: mready.decr();
> 136: VirtualMachine vm =
> VirtualMachine.attach(String.valueOf(ProcessHandle.current().pid()));
I think sleep is needed here so threads which should be unmounted have time to
unmount before attach.
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
line 141:
> 139: log("main: completedNo: " + completedNo);
> 140: attached = true;
> 141: for (Thread t : threads) {
AFAIU threads in 3rd group (TCNT3) should be unmounted (with
LockSupport.parkNanos) before they are interrupted.
Then we need sleep here
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
line 149:
> 147: for (int sleepNo = 0; sleepNo < 10 && threadEndCount() <
> THREAD_CNT; sleepNo++) {
> 148: log("main: wait iter: " + sleepNo);
> 149: Thread.sleep(100);
sleep(1000)? (comment before the loop tells about 10 secs)
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/libVThreadEventTest.cpp
line 37:
> 35:
> 36: namespace {
> 37: std::mutex lock;
This mutex is only to make access to counters atomic.
It would be clearer to make counters std::atomic and remove the mutex
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317795256
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317794334
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317796891
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317811234
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317804389
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317802305
Re: Question on why sun.management MBeans are not exported?
On 06/09/2023 16:17, Volker Simonis wrote: : I'm familiar with JEP 260. But wouldn't you agree that an "encapsulated" monitoring API is an oxymoron? A monitoring API is by design intended for external usage and completely useless to the platform itself. There's no single usage of the "sun.management" MBeans in the JDK itself (except for jconsole where the encapsulation broke it). My assumption is that the corresponding MBeans in "sun.management" are there for historic reasons (added in JDK 1.5) and would have made much more sense in "com.sun.management" package. But I doubt that they can be classified in the "internal implementation details of the JDK and never intended for external use² category of JEP 260. It's left over from experiments on exposing some internal metrics, I think during JDK 5. It's code that should probably have been removed a long time ago. -Alan
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Wed, 6 Sep 2023 16:49:39 GMT, Chris Plummer wrote: > > I wonder if this is the right thing to do for the hprof files. I believe > > they originated from some hprof tools that we no longer ship. 3rd parties > > might choose to integrate them into their own tools. > > Do you think I should revert them? They are test classes now. If someone does want to copy them into their own repo then I assume they can take it from an old repo, maybe from when the "hat" tool existed. - PR Comment: https://git.openjdk.org/jdk/pull/15573#issuecomment-1708828207
Re: Question on why sun.management MBeans are not exported?
On 9/6/23 8:17 AM, Volker Simonis wrote: Anyway, if you classify the MBeans in "sun.management" as non-critical internal APIs (with respect to JEP 260) but without any "internal" usage, than we should really remove them, right, because an internal API without any internal usage doesn't make any sense? We added these HotSpot internal MBeans in JDK 5 to expose the internal metrics. Most of these internal metrics are exposed via jstat tool too. We didn't receive much feedback regarding these HotSpot internal MBeans. Removing them is fine and good cleanup effort. Mandy I'll then try to come up with a proposal to port some of the more useful MBeans functionality in "sun.management" to "com.sun.management". Thank you and best regards, Volker : So to cut a long story short, I see several options: 1. Publicly export sun.management and restore the JDK 8 (or pre JDK 16) behavior. This would certainly require some polishing (e.g. some of the corresponding JVM functionality has already been removed [1]) but I think it could still be quite useful. 2. Port the useful functionality from the "sun.management" MBeans to corresponding "com.sun.management" MBeans and remove the "sun.management" MBeans. 3. Remove the "sun.management" MBeans without substitution. What do you think? If there are JDK-specific or HotSpot VM specific features where there is a compelling case for a management interface then com.sun.management is good place to prototype new APIs. You may already be familiar with com.sun.management.HotSpotDiagnosticMBean. -Alan
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Wed, 6 Sep 2023 16:06:29 GMT, Erik Joelsson wrote: > > I wonder if this is the right thing to do for the hprof files. I believe > > they originated from some hprof tools that we no longer ship. 3rd parties > > might choose to integrate them into their own tools. > > Do you think I should revert them? I'm not sure. I think you need to consult someone with expertise in this area. - PR Comment: https://git.openjdk.org/jdk/pull/15573#issuecomment-1708757719
Re: RFR: 8315097: Rename createJavaProcessBuilder [v3]
On Wed, 30 Aug 2023 09:23:55 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > fix static import I think you are missing the point. If you take a look at [the parent bug of the sub task](https://bugs.openjdk.org/browse/JDK-8314823) you can see that the problem described is *not* that people are using `createTestJvm` in error. The problem is that they are (or possibly are) using `createJavaProcessBuilder` in error. Thus renaming `createTestJvm` might help a little at most for this specific problem. Renaming `createJavaProcessBuilder` most probably helps *more*. I guess the alternative of forcing the user to make a choice using an enum value will help even more. - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1708705105
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 23:12:51 GMT, Chris Plummer wrote: > I wonder if this is the right thing to do for the hprof files. I believe they > originated from some hprof tools that we no longer ship. 3rd parties might > choose to integrate them into their own tools. Do you think I should revert them? - PR Comment: https://git.openjdk.org/jdk/pull/15573#issuecomment-1708676439
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 22:49:41 GMT, Erik Joelsson wrote: > There are a number of files in the `test` directory that have an incorrect > copyright header, which includes the "classpath" exception text. This patch > removes that text from all test files that I could find it in. I did this > using a combination of `sed` and `grep`. Reviewing this patch is probably > easier using the raw patch file or a suitable webrev format. > > It's my assumption that these headers were introduced by mistake as it's > quite easy to copy the wrong template when creating new files. Thanks for fixing! - Marked as reviewed by iris (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15573#pullrequestreview-1613704179
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v4]
> The `find` method now is
> ```C++
> template
> int find(T* token, bool f(T*, E)) const {
> ...
>
> Any other functions which use this are also changed.
> Local linux-x64-debug hotspot:tier1 passed. Mach5 tier1 build on linux and
> Windows passed.
Afshin Zafari has updated the pull request incrementally with one additional
commit since the last revision:
changed the `E` param of find methods to `const E&`.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/15418/files
- new: https://git.openjdk.org/jdk/pull/15418/files/266f6feb..d70f6141
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=15418&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=15418&range=02-03
Stats: 15 lines in 9 files changed: 0 ins; 0 del; 15 mod
Patch: https://git.openjdk.org/jdk/pull/15418.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15418/head:pull/15418
PR: https://git.openjdk.org/jdk/pull/15418
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing [v3]
> In the virtual thread implementation, thread identity switches to the carrier > before freezing and switches back to the virtual thread after thawing. This > was a forced move due to issues getting JVMTI to work with virtual threads. > JVMTI can now hide events during transitions so we can invert the sequence > back to mounting before running the continuation, unmounting after freezing, > and re-mounting after thawing. This sequence is important for future changes > that will initiate the freezing from the VM. > > The change requires an update to the JFR thread sampler to skip sampling when > it samples during a transition. > > Testing: tier1-5 Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Move transition check to JfrVframeStream ctor - Merge - Check for transition during stack walk for synchronous case - Merge - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/15492/files - new: https://git.openjdk.org/jdk/pull/15492/files/18f63cbd..507ae919 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15492&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15492&range=01-02 Stats: 6688 lines in 179 files changed: 5237 ins; 969 del; 482 mod Patch: https://git.openjdk.org/jdk/pull/15492.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15492/head:pull/15492 PR: https://git.openjdk.org/jdk/pull/15492
Re: Question on why sun.management MBeans are not exported?
On Wed, Sep 6, 2023 at 3:47 PM Alan Bateman wrote: > > On 06/09/2023 14:02, Volker Simonis wrote: > > : > > > > I wonder why "sun.management" was encapsulated in the first place? I > > understand that it is not an "officially supported" API, but I find it > > still quite useful. > sun.management.* is JDK internal so not something for code outside the > JDK to use directly. The only sun.* packages that are exported to all > modules are the "critical internal APIs" in the jdk.unsupported module. > JEP 260 has the details. I'm familiar with JEP 260. But wouldn't you agree that an "encapsulated" monitoring API is an oxymoron? A monitoring API is by design intended for external usage and completely useless to the platform itself. There's no single usage of the "sun.management" MBeans in the JDK itself (except for jconsole where the encapsulation broke it). My assumption is that the corresponding MBeans in "sun.management" are there for historic reasons (added in JDK 1.5) and would have made much more sense in "com.sun.management" package. But I doubt that they can be classified in the "internal implementation details of the JDK and never intended for external use² category of JEP 260. Anyway, if you classify the MBeans in "sun.management" as non-critical internal APIs (with respect to JEP 260) but without any "internal" usage, than we should really remove them, right, because an internal API without any internal usage doesn't make any sense? I'll then try to come up with a proposal to port some of the more useful MBeans functionality in "sun.management" to "com.sun.management". Thank you and best regards, Volker > > > > : > > > > So to cut a long story short, I see several options: > > > > 1. Publicly export sun.management and restore the JDK 8 (or pre JDK > > 16) behavior. This would certainly require some polishing (e.g. some > > of the corresponding JVM functionality has already been removed [1]) > > but I think it could still be quite useful. > > 2. Port the useful functionality from the "sun.management" MBeans to > > corresponding "com.sun.management" MBeans and remove the > > "sun.management" MBeans. > > 3. Remove the "sun.management" MBeans without substitution. > > > > What do you think? > If there are JDK-specific or HotSpot VM specific features where there is > a compelling case for a management interface then com.sun.management is > good place to prototype new APIs. You may already be familiar with > com.sun.management.HotSpotDiagnosticMBean. > > -Alan
Re: Question on why sun.management MBeans are not exported?
On 06/09/2023 14:02, Volker Simonis wrote: : I wonder why "sun.management" was encapsulated in the first place? I understand that it is not an "officially supported" API, but I find it still quite useful. sun.management.* is JDK internal so not something for code outside the JDK to use directly. The only sun.* packages that are exported to all modules are the "critical internal APIs" in the jdk.unsupported module. JEP 260 has the details. : So to cut a long story short, I see several options: 1. Publicly export sun.management and restore the JDK 8 (or pre JDK 16) behavior. This would certainly require some polishing (e.g. some of the corresponding JVM functionality has already been removed [1]) but I think it could still be quite useful. 2. Port the useful functionality from the "sun.management" MBeans to corresponding "com.sun.management" MBeans and remove the "sun.management" MBeans. 3. Remove the "sun.management" MBeans without substitution. What do you think? If there are JDK-specific or HotSpot VM specific features where there is a compelling case for a management interface then com.sun.management is good place to prototype new APIs. You may already be familiar with com.sun.management.HotSpotDiagnosticMBean. -Alan
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 22:49:41 GMT, Erik Joelsson wrote: > There are a number of files in the `test` directory that have an incorrect > copyright header, which includes the "classpath" exception text. This patch > removes that text from all test files that I could find it in. I did this > using a combination of `sed` and `grep`. Reviewing this patch is probably > easier using the raw patch file or a suitable webrev format. > > It's my assumption that these headers were introduced by mistake as it's > quite easy to copy the wrong template when creating new files. Client changes look good. I've looked through all the files, other files look good too. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15573#pullrequestreview-1613295743
Question on why sun.management MBeans are not exported?
Hi,
I recently looked for an easy way to get the CPU time spent by
JIT-compiler and GC threads in Java (e.g exported by IBM J9's
JvmCpuMonitorMXBean [0]). An easy way to achieve this is in OpenJDK is
by using the "sun.management.HotspotInternal" MBean which exports the
"sun.management:type=HotspotThreading" MBean with the attributes
InternalThreadCount/InternalThreadCpuTimes (among other useful HotSpot
internal counters and metrics).
Up until JDK 16/17 the usage of the "sun.management.HotspotInternal"
MBean was straightforward, although it resulted in an illegal
reflective access warning since JDK 9. Since JDK 17 its usage requires
the " --add-exports java.management/sun.management=ALL-UNNAMED" for
the monitored JVM.
I wonder why "sun.management" was encapsulated in the first place? I
understand that it is not an "officially supported" API, but I find it
still quite useful. If we really don't want to export it, I wonder why
we are maintaining it at all (we even have JTreg tests for it under
"test/jdk/sun/management"), because in the current configuration it
isn't particularly useful.
Notice that jconsole supports the "sun.management.HotspotInternal"
MBean if started with "J-Djconsole.showUnsupported=true" and the
"java.management" module kind of tries to support jconsole with the
following exports:
exports sun.management to
jdk.jconsole,
jdk.management,
jdk.management.agent;
However, that doesn't help even if jconsole monitors itself (for the
general case, where jconsole monitors a different JVM process it can't
work anyway). With JDK 16 and "--illegal-access=debug" we can see why:
WARNING: Illegal reflective access by sun.reflect.misc.Trampoline to
method sun.management.HotspotThreadMBean.getInternalThreadCount()
at sun.reflect.misc.Trampoline.invoke(MethodUtil.java:71)
at java.base/sun.reflect.misc.MethodUtil.invoke(MethodUtil.java:260)
at
java.management/com.sun.jmx.mbeanserver.StandardMBeanIntrospector.invokeM2(StandardMBeanIntrospector.java:112)
at
java.management/com.sun.jmx.mbeanserver.StandardMBeanIntrospector.invokeM2(StandardMBeanIntrospector.java:46)
at
java.management/com.sun.jmx.mbeanserver.MBeanIntrospector.invokeM(MBeanIntrospector.java:237)
at
java.management/com.sun.jmx.mbeanserver.PerInterface.getAttribute(PerInterface.java:83)
at
java.management/com.sun.jmx.mbeanserver.MBeanSupport.getAttribute(MBeanSupport.java:206)
at
java.management/com.sun.jmx.mbeanserver.MBeanSupport.getAttributes(MBeanSupport.java:213)
at
java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.getAttributes(DefaultMBeanServerInterceptor.java:701)
at
java.management/com.sun.jmx.mbeanserver.JmxMBeanServer.getAttributes(JmxMBeanServer.java:705)
Notice that 'sun.reflect.misc.Trampoline' is loaded by a custom class
loader ('java.base/sun.reflect.misc.MethodUtil') and not considered
part of the base module.
So to cut a long story short, I see several options:
1. Publicly export sun.management and restore the JDK 8 (or pre JDK
16) behavior. This would certainly require some polishing (e.g. some
of the corresponding JVM functionality has already been removed [1])
but I think it could still be quite useful.
2. Port the useful functionality from the "sun.management" MBeans to
corresponding "com.sun.management" MBeans and remove the
"sun.management" MBeans.
3. Remove the "sun.management" MBeans without substitution.
What do you think?
Thank you and best regards,
Volker
[0]
https://www.ibm.com/docs/en/sdk-java-technology/8?topic=interfaces-language-management
[1] https://bugs.openjdk.org/browse/JDK-8134607
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 22:49:41 GMT, Erik Joelsson wrote: > There are a number of files in the `test` directory that have an incorrect > copyright header, which includes the "classpath" exception text. This patch > removes that text from all test files that I could find it in. I did this > using a combination of `sed` and `grep`. Reviewing this patch is probably > easier using the raw patch file or a suitable webrev format. > > It's my assumption that these headers were introduced by mistake as it's > quite easy to copy the wrong template when creating new files. jmx, jndi, and net changes LGTM - PR Review: https://git.openjdk.org/jdk/pull/15573#pullrequestreview-1613147541
Re: RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX
On Wed, 6 Sep 2023 08:18:45 GMT, JoKern65 wrote: > After push of [JDK-8307478](https://bugs.openjdk.org/browse/JDK-8307478) , > the following test started to fail on AIX : > com/sun/tools/attach/warnings/DynamicLoadWarningTest.java; > The problem was described in > [JDK-8309549](https://bugs.openjdk.org/browse/JDK-8309549) with a first try > of a fix. > A second fix via [JDK-8310191](https://bugs.openjdk.org/browse/JDK-8310191) > was necessary. > Both fixes just disable the specific subtest on AIX, without correction of > the root cause. > The root cause is, that dlopen() on AIX returns different handles every time, > even if you load a library twice. There is no official AIX API available to > get this information on a different way. > My proposal is, to use the stat64x API with the fields st_device and > st_inode. After a dlopen() the stat64x() API is called additionally to get > this information which is then stored parallel to the library handle in the > jvmtiAgent. For AIX we then can compare these values instead of the library > handle and get the same functionality as on linux. Just to point out that AIX behavior is not wrong, the spec is clear that "it is implementation specific as to whether a warning is printed when attempting to start the same agent a second or subsequent time". Just pointing it out as you have the option to avoid this complexity if you want. The cited JBS issues are because the original tests for JEP 451 assumed that there would be additional warnings when the same agent was loaded/started several times. - PR Comment: https://git.openjdk.org/jdk/pull/15583#issuecomment-1707909744
RFR: JDK-8315706: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java real fix for failure on AIX
After push of [JDK-8307478](https://bugs.openjdk.org/browse/JDK-8307478) , the following test started to fail on AIX : com/sun/tools/attach/warnings/DynamicLoadWarningTest.java; The problem was described in [JDK-8309549](https://bugs.openjdk.org/browse/JDK-8309549) with a first try of a fix. A second fix via [JDK-8310191](https://bugs.openjdk.org/browse/JDK-8310191) was necessary. Both fixes just disable the specific subtest on AIX, without correction of the root cause. The root cause is, that dlopen() on AIX returns different handles every time, even if you load a library twice. There is no official AIX API available to get this information on a different way. My proposal is, to use the stat64x API with the fields st_device and st_inode. After a dlopen() the stat64x() API is called additionally to get this information which is then stored parallel to the library handle in the jvmtiAgent. For AIX we then can compare these values instead of the library handle and get the same functionality as on linux. - Commit messages: - JDK-8315706 Changes: https://git.openjdk.org/jdk/pull/15583/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15583&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8315706 Stats: 121 lines in 5 files changed: 118 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15583.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15583/head:pull/15583 PR: https://git.openjdk.org/jdk/pull/15583
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 23:15:53 GMT, Jonathan Gibbons wrote: > One has to wonder about the `**/*_OLD.java` files, but that would be a > different cleanup The IBM double byte charsets were re-implemented in JDK 7. I think the old implementations moved to the test tree so it could be used to test the new/replacement implementations. - PR Comment: https://git.openjdk.org/jdk/pull/15573#issuecomment-1707845577
Integrated: 8315648: Add test for JDK-8309979 changes
On Mon, 4 Sep 2023 14:18:55 GMT, Roman Marchenko wrote: > This change added a simple check in > test/hotspot/jtreg/serviceability/sa/ClhsdbDumpclass.java if BootstrapMethods > attribute was dumped. (similar to test changes in PR #14556) This pull request has now been integrated. Changeset: a258fc44 Author:Roman Marchenko Committer: Yuri Nesterenko URL: https://git.openjdk.org/jdk/commit/a258fc443f6a119a122814f6c69e489ed0513856 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8315648: Add test for JDK-8309979 changes Reviewed-by: cjplummer - PR: https://git.openjdk.org/jdk/pull/15561
