Re: RFR: 8335291: Problem list all SA core file tests on macosx-aarch64 due to JDK-8318754 [v2]
On Mon, 1 Jul 2024 08:16:07 GMT, Chris Plummer wrote: >> On macosx-aarch64, sometimes the generated core file does not contain all >> valid memory. This causes SA tests to fail with various address related java >> exceptions. There's nothing SA can do to work around the problem, and these >> failures over time have been just too noisy, so I'm problem listing all the >> SA core files tests on macosx-aarch64. Note they are already problem listed >> on macosx-x64 dues to >> [JDK-8267433](https://bugs.openjdk.org/browse/JDK-8267433) (the tests time >> out while generating the core dump because it takes way too long), so >> unfortunately this means no more core file testing on macosx, at least for >> now. >> >> Tested with tier1 and running all tier2 and tier5 SA test tasks. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Use proper CR in problem list Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19950#pullrequestreview-2154516441
Re: RFR: 8335370: Fix -Wzero-as-null-pointer-constant warning in jvmti_common.hpp
On Sun, 30 Jun 2024 22:15:34 GMT, Kim Barrett wrote: > Please review this trivial change to the print_method function in > test/lib/jdk/test/lib/jvmti/jvmti_common.hpp. It was passing 0 as the FILE* > argument to fflush, triggering -Wzero-as-null-pointer-constant if that warning > is enabled. The change is to replace that 0 with nullptr. > > This removes about 10% of the -Wzero-as-null-pointer-constant warnings when > building with that option. > > Testing: mach5 tier1 Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19962#pullrequestreview-2154513850
Re: RFR: 8333361: ubsan, test : libHeapMonitorTest.cpp:518:9: runtime error: null pointer passed as argument 2, which is declared to never be null
On Thu, 20 Jun 2024 11:58:23 GMT, Matthias Baesken wrote: > The following issue has been observed when running > serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorThreadTest (and some > other :tier1 tests) > on Linux with ubsan enabled binaries : > > test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.cpp:518:9: > runtime error: null pointer passed as argument 2, which is declared to never > be null > #0 0x80388020 in event_storage_augment_storage > test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.cpp:518 > #1 0x80388020 in event_storage_add > test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/libHeapMonitorTest.cpp:557 > #2 0x85e695fc in JvmtiExport::post_sampled_object_alloc(JavaThread*, > oopDesc*) src/hotspot/share/prims/jvmtiExport.cpp:2926 > #3 0x85e558b8 in > JvmtiObjectAllocEventCollector::generate_call_for_allocated() > src/hotspot/share/prims/jvmtiExport.cpp:3074 > #4 0x85e56c14 in > JvmtiSampledObjectAllocEventCollector::~JvmtiSampledObjectAllocEventCollector() > src/hotspot/share/prims/jvmtiExport.cpp:3171 > #5 0x85e56c14 in > JvmtiSampledObjectAllocEventCollector::~JvmtiSampledObjectAllocEventCollector() > src/hotspot/share/prims/jvmtiExport.cpp:3166 > #6 0x862ace34 in > MemAllocator::Allocation::notify_allocation_jvmti_sampler() > src/hotspot/share/gc/shared/memAllocator.cpp:196 > #7 0x862af7a4 in MemAllocator::Allocation::~Allocation() > src/hotspot/share/gc/shared/memAllocator.cpp:87 > #8 0x862af7a4 in MemAllocator::allocate() const > src/hotspot/share/gc/shared/memAllocator.cpp:356 > #9 0x86dca4dc in CollectedHeap::array_allocate(Klass*, unsigned long, > int, bool, JavaThread*) > src/hotspot/share/gc/shared/collectedHeap.inline.hpp:41 > #10 0x86dca4dc in TypeArrayKlass::allocate_common(int, bool, > JavaThread*) src/hotspot/share/oops/typeArrayKlass.cpp:93 > #11 0x86dca4dc in TypeArrayKlass::allocate_common(int, bool, > JavaThread*) src/hotspot/share/oops/typeArrayKlass.cpp:89 > #12 0x857f35c8 in InterpreterRuntime::newarray(JavaThread*, > BasicType, int) src/hotspot/share/interpreter/interpreterRuntime.cpp:232 > #13 0x6b094cf4 () Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19804#pullrequestreview-2131088045
RFR: 8334169: Long arguments of attach operation are silently truncated on Windows
The change fixes a bug in Attach API implementation on Windows when argument(s) are longer than 1023 bytes testing: test/hotspot/jtreg/serviceability/attach, test/jdk/com/sun/tools/attach on Oracle supported platforms - Commit messages: - JDK-8334168 Changes: https://git.openjdk.org/jdk/pull/19780/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19780=00 Issue: https://bugs.openjdk.org/browse/JDK-8334169 Stats: 180 lines in 2 files changed: 172 ins; 1 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19780.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19780/head:pull/19780 PR: https://git.openjdk.org/jdk/pull/19780
Re: RFR: 8333730: ubsan: FieldIndices/libFieldIndicesTest.cpp:276:11: runtime error: null pointer passed as argument 2, which is declared to never be null
On Tue, 11 Jun 2024 13:00:44 GMT, Matthias Baesken wrote: > When running HS :tier1 tests with ubsan-enabled binaries, the following issue > is reported : > test > serviceability/jvmti/FollowReferences/FieldIndices/FieldIndicesTest.jtr > > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp:276:11: > runtime error: null pointer passed as argument 2, which is declared to never > be null > #0 0x7efea442e379 in Klass::explore_interfaces(JNIEnv_*) > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp:276 > #1 0x7efea443280d in Klass::explore(JNIEnv_*, _jclass*) > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp:322 > #2 0x7efea4433adf in Object::explore(JNIEnv_*, _jobject*) > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp:349 > #3 0x7efea443443b in Java_FieldIndicesTest_prepare > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp:489 > #4 0x7efe7f8d1e7b () Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19657#pullrequestreview-245359
Re: RFR: 8333841: Add more logging into setfldw001 tests.
On Sat, 8 Jun 2024 17:00:04 GMT, Leonid Mesnik wrote: > Tests > SetFieldAccessWatch/setfldw001 > SetFieldModificationWatch/setfmodw001 > intermittently fail with Xcomp. I was unable to reproduce the problem. > The fix adds more checks and variants with jvmti logging. > > The goal is to understand why the test fails. > 1. Confirm that the event is not sent. Currently, the test doesn't differ > between sending "NULL" event and not sending an event at all. > 2. Check if the interpreter-only mode is switched too late in Thread-1. The > jvmti logging shows when field events are enabled and when each thread > actually switches to interpreter-only and enables event handling. > > The plan is to try to reproduce the failure and remove '@test id=logging' > after https://bugs.openjdk.org/browse/JDK-8205957 is fixed. Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19612#pullrequestreview-2108656874
Re: RFR: 8333813: Seviceability tests fail due to stderr containing Temporarily processing option UseNotificationThread
On Fri, 7 Jun 2024 19:56:03 GMT, Chris Plummer wrote: > Allow warning messages such as the following to appear in stderr: > > Java HotSpot(TM) 64-Bit Server VM warning: Temporarily processing option > UseNotificationThread; support is scheduled for removal in 24.0 > > Tested by running CI tier5, which reproduced the issue. Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19606#pullrequestreview-2105446403
Integrated: 8333680: com/sun/tools/attach/BasicTests.java fails with "SocketException: Permission denied: connect"
On Thu, 6 Jun 2024 02:12:11 GMT, Alex Menkov wrote: > The fix updates com/sun/tools/attach/BasicTests.java to listen and connect > using loopback addresses > > Testing: run the test on all Oracle-supported platforms This pull request has now been integrated. Changeset: 17bd483f Author: Alex Menkov URL: https://git.openjdk.org/jdk/commit/17bd483ff01e463cef45824f0c1296a8f3e782c8 Stats: 12 lines in 3 files changed: 6 ins; 0 del; 6 mod 8333680: com/sun/tools/attach/BasicTests.java fails with "SocketException: Permission denied: connect" Reviewed-by: sspitsyn, kevinw, lmesnik - PR: https://git.openjdk.org/jdk/pull/19571
Re: RFR: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic
On Fri, 7 Jun 2024 10:31:22 GMT, Jaikiran Pai wrote:
> Can I please get a review of this test-only change which fixes an issue that
> was introduced due to the refactoring that we did in
> https://bugs.openjdk.org/browse/JDK-8333130? This PR addresses the failure
> reported in https://bugs.openjdk.org/browse/JDK-8333756.
>
> The `NativeMethodPrefixApp` test uses a `javaagent` `NativeMethodPrefixAgent`
> which modifies the name of the native methods using the
> `java.lang.instrument.Instrumentation` instance:
>
> public static void premain (String agentArgs, Instrumentation instArg) {
> inst = instArg;
> System.out.println("Premain");
>
> ...
> instArg.setNativeMethodPrefix(t0, "wrapped_tr0_");
> instArg.setNativeMethodPrefix(t1, "wrapped_tr1_");
> instArg.setNativeMethodPrefix(t2, "wrapped_tr2_");
>
>
> The Hotspot VM allows for methods on a class to be annotated with an (VM
> internal) `jdk.internal.vm.annotation.IntrinsicCandidate` annotation. When a
> class that contains any methods that are annotated with `@IntrinsicCandidate`
> is loaded, the VM checks that the corresponding method(s) have an intrinsic
> available. It uses the method name to check for the presence of the
> intrinsic. In the absence of an intrinsic for a `@IntrinsicCandidate` method,
> the VM throws an error and exits. This behaviour is controlled by the
> `-XX:+/-CheckIntrinsics` option. By default that option is enabled, implying
> that an error will be thrown if the intrinsic isn't found.
>
> In the case where/when this test fails, it so happens that the JVM loads a
> class which has a `@IntrinsicCandidate` on a `native` Java method. For
> example, on the failing host, I could see this class loading sequence:
>
>
> tr2: Loading java/util/Date
> tr1: Loading java/util/Date
> tr0: Loading java/util/Date
> tr2: Loading sun/util/calendar/CalendarSystem
> tr1: Loading sun/util/calendar/CalendarSystem
> tr0: Loading sun/util/calendar/CalendarSystem
> tr2: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr1: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> tr0: Loading sun/util/calendar/CalendarSystem$GregorianHolder
> ...
> tr2: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr1: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr0: Loading sun/util/calendar/ZoneInfoFile$Checksum
> tr2: Loading java/util/zip/Checksum
> tr1: Loading java/util/zip/Checksum
> tr0: Loading java/util/zip/Checksum
> tr2: Loading java/util/zip/CRC32
> tr1: Loading java/util/zip/CRC32
> tr0: Loading java/util/zip/CRC32
> Method [java.util.zip.CRC32.wrapped_tr2_update(II)I] is annotated with
> @IntrinsicCandidate, but no...
Marked as reviewed by amenkov (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/19595#pullrequestreview-2105238174
RFR: 8333680: com/sun/tools/attach/BasicTests.java fails with "SocketException: Permission denied: connect"
The fix updates com/sun/tools/attach/BasicTests.java to listen and connect using loopback addresses Testing: run the test on all Oracle-supported platforms - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/19571/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19571=00 Issue: https://bugs.openjdk.org/browse/JDK-8333680 Stats: 12 lines in 3 files changed: 6 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19571.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19571/head:pull/19571 PR: https://git.openjdk.org/jdk/pull/19571
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v6]
On Tue, 4 Jun 2024 12:31:13 GMT, Jaikiran Pai wrote: >> My guess is these tests were timed out when executed with some specific >> options like -Xint, -Xcomp or any other that can slow down the execution >> (e.g. DeoptimizeALot). > > I have now updated the PR to use the same timeout that was used for the > `shell` test before the changes in this PR. I think `timeout`s are not needed for the refactored tests. Per JDK-6528548 the shell action has timed out using a network mounted JDK (MakeJAR2.sh run `javac` 3 times and `jar` 1 time). But I don't see big problem here - up to the author to keep or remove them. - PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1626468824
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v6]
On Tue, 4 Jun 2024 12:35:33 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which addresses >> https://bugs.openjdk.org/browse/JDK-8333130? >> >> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` >> under `test/jdk/java/lang/instrument/` which launch the app/test with a >> `-javaagent:` pointing to a test specific jar. The test, launched with >> that `javaagent`, then verifies the test specific details. >> >> In their current form, in order to construct the agent `.jar` and make it >> available to the jtreg test that's launched, they `@run` a jtreg `shell` >> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar >> file. The contents of the script is relatively straightforward - it compiles >> (using `javac`) some boot classpath classes, some agent specific classes and >> then generates a jar file with the agent classes and a `MANIFEST.MF` which >> points to the boot classpath classes along with test specific manifest >> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass >> `--enable-preview --release 23` since one of the existing agent class was >> updated to use the ClassFile API PreviewFeature >> (https://github.com/openjdk/jdk/pull/13009). >> >> This generated agent jar then is passed as `-javaagent:` to the subsequent >> `@run main` test which does the actual testing. >> >> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there >> to create a jar file that's needed by the actual test. Within the JDK we >> have several tests which compile other classes and generate jar files using >> in-process test libraries and the `java.util.spi.ToolProvider` >> implementations. These 2 tests too can be updated to use a similar >> technique, to completely get rid of the `MakeJAR2.sh`. Removing the >> `MakeJAR2.sh` will simplify the ability to pass around value for `--release` >> when using `--enable-preview`. >> >> The commit in this PR updates these 2 tests to use `@run driver` which then >> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and >> generates the agent jar file and then finally launching the test process. As >> part of the update, I did not retain the `@author` tag from the 2 test >> definitions, since it's my understanding that we no longer use those. I will >> add them back if we should retain them. >> >> The tests continue to pass locally with this change. I've also triggered >> tier testing in our CI for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Serguei's suggestion - use a higher timeout for the test to match what was > being used for the shell test Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19495#pullrequestreview-2097164263
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v4]
On Mon, 3 Jun 2024 22:55:24 GMT, Serguei Spitsyn wrote: >> Please, review the following `interp-only` issue related to carrier threads. >> There are 3 problems fixed here: >> - The `EnterInterpOnlyModeClosure::do_threads` is taking the >> `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect >> when we have a deal with a carrier thread. The target state is known at the >> point when the `HandshakeClosure` is set, so the fix is to pass it as a >> constructor parameter. >> - The `state->is_pending_interp_only_mode())` was processed at mounts only >> but it has to be processed for unmounts as well. >> - The test >> `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp` >> has a wrong assumption that there can't be `MethodExit` event on the >> carrier thread when the function `breakpoint_hit1` is being executed. >> However, it can happen if the virtual thread gets unmounted. >> >> The fix also includes new test case >> `vthread/CarrierThreadEventNotification` developed by Patricio. >> >> Testing: >> - Ran new test case locally >> - Ran mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: get rid of unneeded casts in new test Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19438#pullrequestreview-2095027750
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v3]
On Sat, 1 Jun 2024 07:41:29 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this test-only change which addresses
>> https://bugs.openjdk.org/browse/JDK-8333130?
>>
>> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp`
>> under `test/jdk/java/lang/instrument/` which launch the app/test with a
>> `-javaagent:` pointing to a test specific jar. The test, launched with
>> that `javaagent`, then verifies the test specific details.
>>
>> In their current form, in order to construct the agent `.jar` and make it
>> available to the jtreg test that's launched, they `@run` a jtreg `shell`
>> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar
>> file. The contents of the script is relatively straightforward - it compiles
>> (using `javac`) some boot classpath classes, some agent specific classes and
>> then generates a jar file with the agent classes and a `MANIFEST.MF` which
>> points to the boot classpath classes along with test specific manifest
>> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass
>> `--enable-preview --release 23` since one of the existing agent class was
>> updated to use the ClassFile API PreviewFeature
>> (https://github.com/openjdk/jdk/pull/13009).
>>
>> This generated agent jar then is passed as `-javaagent:` to the subsequent
>> `@run main` test which does the actual testing.
>>
>> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there
>> to create a jar file that's needed by the actual test. Within the JDK we
>> have several tests which compile other classes and generate jar files using
>> in-process test libraries and the `java.util.spi.ToolProvider`
>> implementations. These 2 tests too can be updated to use a similar
>> technique, to completely get rid of the `MakeJAR2.sh`. Removing the
>> `MakeJAR2.sh` will simplify the ability to pass around value for `--release`
>> when using `--enable-preview`.
>>
>> The commit in this PR updates these 2 tests to use `@run driver` which then
>> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and
>> generates the agent jar file and then finally launching the test process. As
>> part of the update, I did not retain the `@author` tag from the 2 test
>> definitions, since it's my understanding that we no longer use those. I will
>> add them back if we should retain them.
>>
>> The tests continue to pass locally with this change. I've also triggered
>> tier testing in our CI for this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Alex's input - simplify the test by using ClassFileInstaller
test/jdk/java/lang/instrument/NativeMethodPrefixApp.java line 86:
> 84: Can-Set-Native-Method-Prefix: true
> 85: """
> 86: + "Boot-Class-Path: " +
> bootClassesDir.toString().replace(File.separatorChar, '/') + "/"
AFAIU Boot-Class-Path is needed only because the agent needs
bootreporter.StringIdCallbackReporter
I don't see much point in separate bootreporter dir.
So we can set Boot-Class-Path to System.getProperty("test.classes") and drop
createBootClassesDir() logic
test/jdk/java/lang/instrument/RetransformApp.java line 81:
> 79: final OutputAnalyzer oa = ProcessTools.executeTestJava(
> 80: "--enable-preview", // due to usage of ClassFile API
> PreviewFeature in the agent
> 81: "-XX:+UnlockDiagnosticVMOptions", "-XX:-CheckIntrinsics",
This `-XX` flags were not present for this test (and I don't think they are
needed for NativeMethodPrefix test too)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1625009748
PR Review Comment: https://git.openjdk.org/jdk/pull/19495#discussion_r1624969611
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Sat, 1 Jun 2024 01:18:35 GMT, Jaikiran Pai wrote: > Hello Alex, what you suggest seems interesting and like you note much more > simpler. I'll try it out and if that will work fine with the use of > `--release` for the `asmlib/Instrumentor.java`. Actually I think you don't need to specify `--release` if you use jtreg "@enablePreview" tag, so "@build" should work fine - PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2143215730
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v3]
On Thu, 30 May 2024 02:41:39 GMT, Serguei Spitsyn wrote: >> test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp >> line 201: >> >>> 199: >>> 200: // need to reset this value after the breakpoint_hit1 >>> 201: received_method_exit_event = JNI_FALSE; >> >> There was a loom-dev email thread regarding this last year. Seems related. I >> had concluded that the way the test was written that no MethodExit event >> should have been received. I'm not sure if I missed something in my analysis >> or if this failure is a result of your changes: >> >> https://mail.openjdk.org/pipermail/loom-dev/2023-August/006059.html >> https://mail.openjdk.org/pipermail/loom-dev/2023-September/006170.html > > Thank you for the comment and links to the discussion. In fact, I've observed > the MethodExit events really posted between the breakpoint hits: `hit1` and > `hit2`. The first one is at the return from the `unmount()` method. I was not > able to prove why they should not be expected. I'm not sure I follow the test logic. Its summary says "Verifies that MethodExit events are delivered on both carrier and virtual threads", but now it just ignores MethodExit requested for carrier thread in breakpoint_hit1. Then there is no sense to request the event on carrier thread. Per the test summary I'd expect the test should test MethodExit for carrier thread, but then java part needs to force unmount - PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623073345
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes [v3]
On Fri, 31 May 2024 23:55:20 GMT, Serguei Spitsyn wrote:
>> Please, review the following `interp-only` issue related to carrier threads.
>> There are 3 problems fixed here:
>> - The `EnterInterpOnlyModeClosure::do_threads` is taking the
>> `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect
>> when we have a deal with a carrier thread. The target state is known at the
>> point when the `HandshakeClosure` is set, so the fix is to pass it as a
>> constructor parameter.
>> - The `state->is_pending_interp_only_mode())` was processed at mounts only
>> but it has to be processed for unmounts as well.
>> - The test
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp`
>> has a wrong assumption that there can't be `MethodExit` event on the
>> carrier thread when the function `breakpoint_hit1` is being executed.
>> However, it can happen if the virtual thread gets unmounted.
>>
>> The fix also includes new test case
>> `vthread/CarrierThreadEventNotification` developed by Patricio.
>>
>> Testing:
>> - Ran new test case locally
>> - Ran mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: refactored def and use of process_pending_interp_only()
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 40:
> 38:
> 39: static const char* CTHREAD_NAME_START = "ForkJoinPool";
> 40: static const size_t CTHREAD_NAME_START_LEN = (int)strlen("ForkJoinPool");
`(int)` cast is not needed
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 58:
> 56: cthreads[ct_cnt++] = jni->NewGlobalRef(thread);
> 57: }
> 58: deallocate(jvmti, jni, (void*)tname);
cast to `void*` is not needed
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 96:
> 94: }
> 95: jvmtiError err = jvmti->Deallocate((unsigned char*)carrier_threads);
> 96: check_jvmti_status(jni, err, "deallocate: error in JVMTI Deallocate
> call");
replace with `deallocate(jvmti, jni, carrier_threads);` ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623060427
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623061692
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1623061890
Re: RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version [v2]
On Fri, 31 May 2024 12:01:14 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this test-only change which addresses
>> https://bugs.openjdk.org/browse/JDK-8333130?
>>
>> There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp`
>> under `test/jdk/java/lang/instrument/` which launch the app/test with a
>> `-javaagent:` pointing to a test specific jar. The test, launched with
>> that `javaagent`, then verifies the test specific details.
>>
>> In their current form, in order to construct the agent `.jar` and make it
>> available to the jtreg test that's launched, they `@run` a jtreg `shell`
>> test. This `shell` test uses the `MakeJAR2.sh` script to generate the jar
>> file. The contents of the script is relatively straightforward - it compiles
>> (using `javac`) some boot classpath classes, some agent specific classes and
>> then generates a jar file with the agent classes and a `MANIFEST.MF` which
>> points to the boot classpath classes along with test specific manifest
>> attributes. In a recent PR the `MakeJAR2.sh` script was updated to pass
>> `--enable-preview --release 23` since one of the existing agent class was
>> updated to use the ClassFile API PreviewFeature
>> (https://github.com/openjdk/jdk/pull/13009).
>>
>> This generated agent jar then is passed as `-javaagent:` to the subsequent
>> `@run main` test which does the actual testing.
>>
>> So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there
>> to create a jar file that's needed by the actual test. Within the JDK we
>> have several tests which compile other classes and generate jar files using
>> in-process test libraries and the `java.util.spi.ToolProvider`
>> implementations. These 2 tests too can be updated to use a similar
>> technique, to completely get rid of the `MakeJAR2.sh`. Removing the
>> `MakeJAR2.sh` will simplify the ability to pass around value for `--release`
>> when using `--enable-preview`.
>>
>> The commit in this PR updates these 2 tests to use `@run driver` which then
>> compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and
>> generates the agent jar file and then finally launching the test process. As
>> part of the update, I did not retain the `@author` tag from the 2 test
>> definitions, since it's my understanding that we no longer use those. I will
>> add them back if we should retain them.
>>
>> The tests continue to pass locally with this change. I've also triggered
>> tier testing in our CI for this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix Boot-Class-Path value for Windows
The fix looks over-complicated.
I think it can be done by something like:
- `@build NativeMethodPrefixAgent bootreporter.StringIdCallback
bootreporter.StringIdCallbackReporter` (maybe `@compile` to specify `--release`)
- use jdk.test.lib.helpers.ClassFileInstaller:
ClassFileInstaller.writeJar("NativeMethodPrefixAgent.jar",
ClassFileInstaller.Manifest.fromString(manifest),
"NativeMethodPrefixAgent",
"asmlib.Instrumentor",
"bootreporter.StringIdCallback",
"bootreporter.StringIdCallbackReporter");
-
PR Comment: https://git.openjdk.org/jdk/pull/19495#issuecomment-2143117731
Integrated: 8320215: HeapDumper can use DumpWriter buffer during merge
On Fri, 19 Apr 2024 00:10:12 GMT, Alex Menkov wrote: > The fix updates HeapMerger to use writer buffer (no need to copy memory, also > writer buffer is 1MB instead of 4KB). > Additionally fixed small issue in FileWriter (looks like `ssize_t` instead of > `size_t` is a typo, the argument should be unsigned) > > Testing: all HeapDump-related tests on Oracle supported platforms This pull request has now been integrated. Changeset: e4fbb15c Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/e4fbb15c6a7b18f1ec66176080404818d3871194 Stats: 22 lines in 3 files changed: 9 ins; 4 del; 9 mod 8320215: HeapDumper can use DumpWriter buffer during merge Reviewed-by: sspitsyn, yyang - PR: https://git.openjdk.org/jdk/pull/18850
Integrated: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly
On Fri, 26 Apr 2024 22:59:43 GMT, Alex Menkov wrote: > Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method > > Testing: tier1-6 This pull request has now been integrated. Changeset: 44c1845a Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/44c1845ae7fdff524d4a60a51362834cfea5c5da Stats: 43 lines in 3 files changed: 3 ins; 11 del; 29 mod 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly Reviewed-by: sspitsyn, cjplummer - PR: https://git.openjdk.org/jdk/pull/18986
Re: RFR: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes
On Tue, 28 May 2024 22:24:53 GMT, Serguei Spitsyn wrote:
> Please, review the following `interp-only` issue related to carrier threads.
> There are 3 problems fixed here:
> - The `EnterInterpOnlyModeClosure::do_threads` is taking the
> `JvmtiThreadState` with the `jt->jvmti_thread_state()` which is incorrect
> when we have a deal with a carrier thread. The target state is known at the
> point when the `HandshakeClosure` is set, so the fix is to pass it as a
> constructor parameter.
> - The `state->is_pending_interp_only_mode())` was processed at mounts only
> but it has to be processed for unmounts as well.
> - The test
> `test/hotspot/jtreg/serviceability/jvmti/vthread/MethodExitTest/libMethodExitTest.cpp`
> has a wrong assumption that there can't be `MethodExit` event on the carrier
> thread when the function `breakpoint_hit1` is being executed. However, it can
> happen if the virtual thread gets unmounted.
>
> The fix also includes new test case `vthread/CarrierThreadEventNotification`
> developed by Patricio.
>
> Testing:
> - Ran new test case locally
> - Ran mach5 tiers 1-6
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/CarrierThreadEventNotification.java
line 2:
> 1: /*
> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
(c) 2024
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 2:
> 1: /*
> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
(c) 2024
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 40:
> 38:
> 39: static const char* CTHREAD_NAME_START = "ForkJoinPool";
> 40: static const int CTHREAD_NAME_START_LEN = (int)strlen("ForkJoinPool");
should be `size_t` (the value is used for `strncmp` which expects `size_t`)
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 44:
> 42: static jint
> 43: get_cthreads(JNIEnv* jni, jthread** cthreads_p) {
> 44: jthread* tested_cthreads = NULL;
Suggestion:
jthread* tested_cthreads = nullptr;
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 44:
> 42: static jint
> 43: get_cthreads(JNIEnv* jni, jthread** cthreads_p) {
> 44: jthread* tested_cthreads = NULL;
This local variable has the same name as global.
I'd suggest to rename the local var or remove it (and the function should set
both `tested_cthreads` and ` cthread_cnt`)
test/hotspot/jtreg/serviceability/jvmti/vthread/CarrierThreadEventNotification/libCarrierThreadEventNotification.cpp
line 91:
> 89: for (int i = 0; i < cthread_cnt; i++) {
> 90: jthread thread = tested_cthreads[i];
> 91: char* tname = get_thread_name(jvmti, jni, thread);
`tname` is not needed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619642814
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619642981
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619643931
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619660102
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619662506
PR Review Comment: https://git.openjdk.org/jdk/pull/19438#discussion_r1619665290
Re: RFR: 8332751: Broken link in VirtualMachine.html
On Wed, 29 May 2024 20:17:30 GMT, Chris Plummer wrote: > Fixed broken javadoc link. I confirmed that it currently is broken as can be > seen in the JDK 22 javadocs: > > https://docs.oracle.com/en%2Fjava%2Fjavase%2F22%2Fdocs%2Fapi%2F%2F/jdk.jdi/com/sun/jdi/VirtualMachine.html#allThreads() > > Click on the "virtual threads" link within the allThreads() description to > see the broken link. > > I verified the fix by building the javadocs and checking that the link now > works. Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19469#pullrequestreview-2086884777
Re: RFR: 8320215: HeapDumper can use DumpWriter buffer during merge
On Wed, 29 May 2024 02:03:42 GMT, Chris Plummer wrote:
>> The fix updates HeapMerger to use writer buffer (no need to copy memory,
>> also writer buffer is 1MB instead of 4KB).
>> Additionally fixed small issue in FileWriter (looks like `ssize_t` instead
>> of `size_t` is a typo, the argument should be unsigned)
>>
>> Testing: all HeapDump-related tests on Oracle supported platforms
>
> src/hotspot/share/services/heapDumper.cpp line 2137:
>
>> 2135: while ((cnt = segment_fs.read(_writer->buffer(), 1,
>> _writer->buffer_size())) != 0) {
>> 2136: _writer->set_position(cnt);
>> 2137: _writer->flush();
>
> Why flush on each iteration instead of just once after you are done with the
> loop?
Standard way to use AbstractDumpWriter is to use
`AbstractDumpWriter::write_XXX` methods (they all call `write_raw(const void*
s, size_t len)` which copies data from the provided memory to
`_writer->buffer()`, and flushes when the buffer is full)
This code uses writer internals - on each iteration we read up to 1MB from
`segment_fs` directly to `_writer->buffer()`, so we need to flush it before
performing next read, otherwise data in the buffer will be overridden.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18850#discussion_r1619412072
Re: RFR: 8320215: HeapDumper can use DumpWriter buffer during merge
On Wed, 29 May 2024 02:52:22 GMT, Yi Yang wrote: > I remember experimenting with different buffer sizes and figuring out that > 4KB was the sweet spot. We could potentially switch to 1MB, but it would be > better if we had some benchmark numbers to back that up. My experiments shown that with the fix time of the merge decreased ~5%. But this is not just change of the buffer size: Before the fix merger (1) read `segment_fs` in 4K chunks, (2) wrote them (copying memory) to `_writer->_buffer` until the buffer is full (i.e. 256 read iterations) and then (3) flush `_writer->_buffer`; With the fix (1) and (2) are replaced with single reading of 1MB directly to `_writer->_buffer` (no memory copy). - PR Comment: https://git.openjdk.org/jdk/pull/18850#issuecomment-2138238839
Re: RFR: 8333013: Update vmTestbase/nsk/share/LocalProcess.java to don't use finalization [v2]
On Tue, 28 May 2024 20:55:30 GMT, Leonid Mesnik wrote: >> The vmTestbase/nsk/share/LocalProcess.java is a wrapper for debuggee >> process. It extends FinalizableObject to kill the debuggee process. >> >> The debuggee process is used by nsk.jdb tests only, see runTest(...) in >> vmTestbase/nsk/share/jdb/JdbTest.java: >> https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/share/jdb/JdbTest.java#L189 >> >> I verified that process is always already terminated when is cleaned during >> VM shutdown hook, >> So the fix is just to remove the finalization. >> >> I also moved LocalProcess into nsk.share.jdb to reduce the visibility of >> class and hardened checks in runTest. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > fixed check Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19418#pullrequestreview-2084083384
Re: RFR: 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject
On Tue, 28 May 2024 23:20:48 GMT, Chris Plummer wrote:
> Fixed "double-checked-locking" bug in `ReferenceImplType.classObject()`.
> Details in the first comment. Tested with the following:
> - com/sun/jdi
> - nsk/jdi
> - nsk/jdwp
> - nsk/jdb
Marked as reviewed by amenkov (Reviewer).
src/jdk.jdi/share/classes/com/sun/tools/jdi/ReferenceTypeImpl.java line 185:
> 183: public String signature() {
> 184: if (signature == null) {
> 185: // Does not need synchronization. Worst case is static info
> is fetched twice
Could you add dot at the end of all updated comments
-
PR Review: https://git.openjdk.org/jdk/pull/19439#pullrequestreview-2084073364
PR Review Comment: https://git.openjdk.org/jdk/pull/19439#discussion_r1618047117
Re: RFR: 8320215: HeapDumper can use DumpWriter buffer during merge
On Fri, 19 Apr 2024 00:10:12 GMT, Alex Menkov wrote: > The fix updates HeapMerger to use writer buffer (no need to copy memory, also > writer buffer is 1MB instead of 4KB). > Additionally fixed small issue in FileWriter (looks like `ssize_t` instead of > `size_t` is a typo, the argument should be unsigned) > > Testing: all HeapDump-related tests on Oracle supported platforms Ping. Can I get second review please. - PR Comment: https://git.openjdk.org/jdk/pull/18850#issuecomment-2136244148
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly [v4]
On Fri, 3 May 2024 01:54:24 GMT, Alex Menkov wrote: >> Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method >> >> Testing: tier1-6 > > Alex Menkov has updated the pull request incrementally with three additional > commits since the last revision: > > - update > - Revert "renamed current_thread to current" > >This reverts commit d5d614bcf0861466acd695296e974d2253f84c9f. > - Revert "renamed current_thread tp current" > >This reverts commit 4602632221044aa754a1bc8d11e7a3e9a0092590. Ping. Can I get second review please. - PR Comment: https://git.openjdk.org/jdk/pull/18986#issuecomment-2136243867
Re: JDK-8330846 - Status check
Hi, I'm not started with the issue, feel free to take it. --alex On 28.05.2024 7:17, Iñigo Mediavilla wrote: Hello, I'm redirecting this message here, that I posted in the beginning to the loom-dev mailing list. I've been looking at possible tickets that I could work on in JBS, and I've run into : JDK-8330846 - Add stacks of mounted virtual threads to the HotSpot thread dump Link: https://bugs.openjdk.org/browse/JDK-8330846 <https://bugs.openjdk.org/browse/JDK-8330846> It seems to be assigned to, Alex Menkov. Alex, is it something that you're working on or that you'd want to work on, or would you be OK if I gave a it a try ? Regards Íñigo Mediavilla Saiz
Integrated: 8331683: Clean up GetCarrierThread
On Sat, 18 May 2024 00:47:59 GMT, Alex Menkov wrote: > JVMTI GetCarrierThread extension function was introduced by loom for testing. > It's used by several tests in hotspot/jtreg/serviceability. > > Testings: tier1..tier6 This pull request has now been integrated. Changeset: 424eb60d Author: Alex Menkov URL: https://git.openjdk.org/jdk/commit/424eb60dedb332237b8ec97e9da6bd95442c0083 Stats: 37 lines in 3 files changed: 4 ins; 27 del; 6 mod 8331683: Clean up GetCarrierThread Reviewed-by: sspitsyn, cjplummer - PR: https://git.openjdk.org/jdk/pull/19289
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v8]
On Fri, 17 May 2024 22:56:25 GMT, Chris Plummer wrote: >> I think expose dbgRawMonitor outside of util.c is wrong (and use >> gdata->rankedMonitors outside of utils.c too). >> If it's really required, it would be better to add functions to >> disable/enable monitor locks. >> But I still don't understand why we need this (and what is JDK-8330193 about) >> Both JVMTI raw monitors and DebugRawMonitor support re-entrance, so this >> thread may enter the locks again (and other threads will wait for the locks >> if they try to enter them). >> And I don't see how we can get deadlocks on dbgRawMonitor, could you please >> elaborate on that. > > The comment above getLocks() pretty much explains it. Before doing any thread > suspension we can't allow any other thread to hold a lock that might be > needed during suspension. This is why getLocks() is used to grab all these > locks in advanced. If they were not grabbed in advanced and one of the locks > was held by another thread that got suspended, then eventually the current > thread (the one that initiated the thread suspension) would block on the > suspended thread, which means it would never get resumed, so we have a > deadlock. If we don't include dbgRawMonitor in this set of locks and we > suspend a thread while it holds it, this prevents the current thread from > doing a debugMonitorEnter on any lock, even ones it already holds. Note we > can't check if the current thread owns a lock without grabbing dbgRawMonitor. > In fact the main purpose of dbgRawMonitor is to allow the current thread to > check if it owns the monitor. > > I don't disagree that it's a bit of a wart that dbgRawMonitor is exposed in > this manner. I just don't see a way around it. I can hide > gdata->rankedMonitors in dbgRawMonitor_lock/unlock() if you want, but I can't > see of way to not export dbgRawMonitor_lock/unlock() in some fashion, > possibly with a different name. Thank you for the explanation, I missed that threads can be suspended during checking for ownerThread. With this lock no other thread (except current) can enter/exit/wait any monitor, but I suppose it's ok as dbgRawMonitor is locked only while current thread suspends required thread(s) - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1605637099
RFR: 8331683: Clean up GetCarrierThread
JVMTI GetCarrierThread extension function was introduced by loom for testing. It's used by several tests in hotspot/jtreg/serviceability. Testings: tier1..tier6 - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/19289/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19289=00 Issue: https://bugs.openjdk.org/browse/JDK-8331683 Stats: 37 lines in 3 files changed: 4 ins; 27 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19289.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19289/head:pull/19289 PR: https://git.openjdk.org/jdk/pull/19289
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v8]
On Thu, 2 May 2024 20:58:27 GMT, Chris Plummer wrote:
>> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 656:
>>
>>> 654: commonRef_lock();
>>> 655: if (gdata->rankedMonitors) {
>>> 656: dbgRawMonitor_lock();
>>
>> What is this call for? I think dbgRawMonitor_lock/dbgRawMonitor_unlock
>> should not be used outside of util.c (I'd remove them from util.h)
>
> After calling getLocks(), there may still be attempts to enter locks. The
> locks should already be held. I filed
> [JDK-8330193](https://bugs.openjdk.org/browse/JDK-8330193) to assert that.
> However, even when entering an already entered lock, we still need to grab
> dbgRawMonitor. I found that out the hard way when there were deadlocks on
> dbgRawMonitor because it was not entered it here.
I think expose dbgRawMonitor outside of util.c is wrong (and use
gdata->rankedMonitors outside of utils.c too).
If it's really required, it would be better to add functions to disable/enable
monitor locks.
But I still don't understand why we need this (and what is JDK-8330193 about)
Both JVMTI raw monitors and DebugRawMonitor support re-entrance, so this thread
may enter the locks again (and other threads will wait for the locks if they
try to enter them).
And I don't see how we can get deadlocks on dbgRawMonitor, could you please
elaborate on that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1605484995
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v6]
On Thu, 9 May 2024 06:10:04 GMT, Chris Plummer wrote: >> This PR adds ranked monitor support to the debug agent. The debug agent has >> a large number of monitors, and it's really hard to know which order to grab >> them in, and for that matter which monitors might already be held at any >> given moment. By imposing a rank on each monitor, we can check to make sure >> they are always grabbed in the order of their rank. Having this in place >> when I was working on >> [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868) would have made >> it much easier to detect a deadlock that was occuring, and the reason for >> it. That's what motivated me to do this work >> >> There were 2 or 3 minor rank issues discovered as a result of these changes. >> I also learned a lot about some of the more ugly details of the locking >> support in the process. >> >> Tested with the following on all supported platforms and with virtual >> threads: >> >> com/sun/jdi >> vmTestbase/nsk/jdi >> vmTestbase/nsk/jdb >> vmTestbase/nsk/jdwp >> >> Still need to run tier2 and tier5. >> >> Details of the changes follow in the first comment. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Flip rank order. Some cleanup and better comments for verifyMonitorRank(). src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1145: > 1143: */ > 1144: > 1145: static jrawMonitorID dbgRawMonitor; As the monitor is used to synchronize access to dbg_monitors array, maybe rename it to something like dbg_monitors_lock? src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1276: > 1274: > 1275: static void > 1276: assertIsCurrentThread(JNIEnv *env, jthread thread, jthread > current_thread) The function gets both threads as arguments, I think `assertIsSameThread` would be more correct name - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1605494862 PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1605498977
Integrated: 8330066: HeapDumpPath and HeapDumpGzipLevel VM options do not mention HeapDumpBeforeFullGC and HeapDumpAfterFullGC
On Tue, 14 May 2024 00:44:44 GMT, Alex Menkov wrote: > The fix updates descriptions of `HeapDumpPath`/`HeapDumpGzipLevel` and > `HeapDumpBeforeFullGC`/`HeapDumpAfterFullGC`/`HeapDumpOnOutOfMemoryError` VM > options This pull request has now been integrated. Changeset: 43b109b1 Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/43b109b111e77d0f7b302debc0d76e4ac7c9ac56 Stats: 8 lines in 1 file changed: 3 ins; 0 del; 5 mod 8330066: HeapDumpPath and HeapDumpGzipLevel VM options do not mention HeapDumpBeforeFullGC and HeapDumpAfterFullGC Reviewed-by: cjplummer, dholmes, yyang - PR: https://git.openjdk.org/jdk/pull/19224
Re: RFR: 8328866: Add raw monitor rank support to the debug agent. [v6]
On Mon, 13 May 2024 18:32:45 GMT, Chris Plummer wrote: >> FWIW Deleting monitors is a tricky business and needs to be done with great >> care. You have to ensure all threads using the monitor are completely done >> with it. Simply locking it first to check it is "unused" is not sufficient. > > This is probably the reason that the debug agent only destroys one of the 20 > or so monitors it creates. I'm not sure how it got to that point. It may have > destroyed more at one point but there were issues, and only one survived > destruction without issue. If debugger attaches to the debuggee, detaches and re-attaches again, are the monitors recreated again? (with rankedMonitor you added an assert if `DebugRawMonitor::monitor` is not null) - PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1599125113
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly [v4]
> Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method > > Testing: tier1-6 Alex Menkov has updated the pull request incrementally with three additional commits since the last revision: - update - Revert "renamed current_thread to current" This reverts commit d5d614bcf0861466acd695296e974d2253f84c9f. - Revert "renamed current_thread tp current" This reverts commit 4602632221044aa754a1bc8d11e7a3e9a0092590. - Changes: - all: https://git.openjdk.org/jdk/pull/18986/files - new: https://git.openjdk.org/jdk/pull/18986/files/46026322..9be24a4a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18986=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18986=02-03 Stats: 122 lines in 2 files changed: 0 ins; 0 del; 122 mod Patch: https://git.openjdk.org/jdk/pull/18986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18986/head:pull/18986 PR: https://git.openjdk.org/jdk/pull/18986
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly [v3]
On Tue, 30 Apr 2024 23:48:02 GMT, Alex Menkov wrote: >> Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method >> >> Testing: tier1-6 > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > renamed current_thread tp current ok, I'll revert last update and rename current_thread to current only in few places where new variable is introduced - PR Comment: https://git.openjdk.org/jdk/pull/18986#issuecomment-2091927214
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v6]
On Tue, 30 Apr 2024 18:54:09 GMT, Alex Menkov wrote: >> The fix makes VM heap dumping parallel by default. >> `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the >> fix affects `HotSpotDiagnosticMXBean.dumpHeap()`, >> `-XX:+HeapDumpBeforeFullGC`, `-XX:+HeapDumpAfterFullGC` and >> `-XX:+HeapDumpOnOutOfMemoryError`. >> >> Testing: >> - manually tested different heap dump scenarios with `-Xlog:heapdump`; >> - tier1,tier2,hs-tier5-svc; >> - all reg.tests that use heap dump. > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > removed space-only change in diagnosticCommand.cpp Currently heap dump is always performed in 2 phases (even if single threaded heap dump is requested or SerialGC is used). This is required to correctly handle unmounted virtual threads. This was implemented in jdk22, so your testing shows regression comparing with jdk21u (which does not dump unmounted vthreads and references from them). Note also that you use `-XX:+HeapDumpAfterFullGC` in your testing and look at total heap dump time. Main advantage of the 2 phase dumping is decreasing STW time (merge phase is performed on the current thread outside of safepoint). I.e. the idea is not to decrease total heap dump time, but to minimize JVM freeze during dumping. But this does not work in case of -XX:+HeapDumpBeforeFullGC and -XX:+HeapDumpAfterFullGC because heap dumping is requested inside safepoint, so merge stage is also performed in safepoint too (I think it's possible to fix it so merge is performed on some other thread, but I'm not sure it worth it). - PR Comment: https://git.openjdk.org/jdk/pull/18748#issuecomment-2091683903
Re: RFR: 8328866: Add raw monitor rank support to the debug agent.
On Wed, 1 May 2024 21:32:46 GMT, Chris Plummer wrote:
> This PR adds ranked monitor support to the debug agent. The debug agent has a
> large number of monitors, and it's really hard to know which order to grab
> them in, and for that matter which monitors might already be held at any
> given moment. By imposing a rank on each monitor, we can check to make sure
> they are always grabbed in the order of their rank. Having this in place when
> I was working on [JDK-8324868](https://bugs.openjdk.org/browse/JDK-8324868)
> would have made it much easier to detect a deadlock that was occuring, and
> the reason for it. That's what motivated me to do this work
>
> There were 2 or 3 minor rank issues discovered as a result of these changes.
> I also learned a lot about some of the more ugly details of the locking
> support in the process.
>
> Tested with the following on all supported platforms and with virtual threads:
>
> com/sun/jdi
> vmTestbase/nsk/jdi
> vmTestbase/nsk/jdb
> vmTestbase/nsk/jdwp
>
> Still need to run tier2 and tier5.
>
> Details of the changes follow in the first comment.
src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 656:
> 654: commonRef_lock();
> 655: if (gdata->rankedMonitors) {
> 656: dbgRawMonitor_lock();
What is this call for? I think dbgRawMonitor_lock/dbgRawMonitor_unlock should
not be used outside of util.c (I'd remove them from util.h)
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1204:
> 1202: // Need to lock during initialization so verifyMonitorRank() can be
> guaranteed that
> 1203: // if the monitor field is set, then the monitor if fully
> initialized. More specifically,
> 1204: // that the rank field has been set.
rank for all monitors can be set in `util_initialize()`:
for (i = 0; i < NUM_DEBUG_RAW_MONITORS; i++) {
dbg_monitors[i]->rank = i;
}
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1231:
> 1229: }
> 1230:
> 1231: dbg_monitor->monitor = NULL;
I think it would be better to protect this with dbgRawMonitor
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1243:
> 1241: error = JVMTI_FUNC_PTR(gdata->jvmti,GetThreadInfo)
> 1242: (gdata->jvmti, thread, );
> 1243: return info.name;
Need to delete JNI reference info.thread_group and info.jthreadGroup (if not
NULL)
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1262:
> 1260: for (i = 0; i < NUM_DEBUG_RAW_MONITORS; i++) {
> 1261: DebugRawMonitor* dbg_monitor = _monitors[i];
> 1262: if (dbg_monitor == NULL) {
Should be `dbg_monitor->monitor`
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1279:
> 1277: return; // This assert is not reliable if the VM is exiting
> 1278: }
> 1279: jthread current_thread = threadControl_currentThread();
Looks like all callers of the `assertIsCurrentThread()` have reference to the
current thread. Maybe pass it as argument?
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1281:
> 1279: jthread current_thread = threadControl_currentThread();
> 1280: if (!isSameObject(env, thread, current_thread)) {
> 1281: tty_message("ERROR: Threads not the same: %p %p\n", thread,
> current_thread);
Not sure the addresses provide useful information. Maybe print thread names?
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1289:
> 1287:
> 1288: static void
> 1289: verifyMonitorRank(JNIEnv *env, DebugRawMonitorRank rank, jthread thread)
please add a comment that the function should be called under dbgRawMonitor
lock (also for other functions which assume this)
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1294:
> 1292: // has a higher rank than the monitor we are about to enter.
> 1293: DebugRawMonitorRank i;
> 1294: for (i = 0; i < NUM_DEBUG_RAW_MONITORS; i++) {
No need to check monitors from the beginning.
Should be enough to check starting from `min(rank,
FIRST_LEAF_DEBUG_RAW_MONITOR)`
src/jdk.jdwp.agent/share/native/libjdwp/util.c line 1470:
> 1468:
> 1469: // Assert that the current thread owns this monitor.
> 1470: assertIsCurrentThread(getEnv(), dbg_monitor->ownerThread);
reading ownerThread (and other) fields without dbgRawMonitor is not MT-safe
(it's not atomic)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r158613
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1588259933
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1586968454
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1586970362
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1586973059
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1586983415
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1586984623
PR Review Comment: https://git.openjdk.org/jdk/pull/19044#discussion_r1588206582
PR Review Comment:
Integrated: 8322043: HeapDumper should use parallel dump by default
On Fri, 12 Apr 2024 02:17:34 GMT, Alex Menkov wrote: > The fix makes VM heap dumping parallel by default. > `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the fix > affects `HotSpotDiagnosticMXBean.dumpHeap()`, `-XX:+HeapDumpBeforeFullGC`, > `-XX:+HeapDumpAfterFullGC` and `-XX:+HeapDumpOnOutOfMemoryError`. > > Testing: > - manually tested different heap dump scenarios with `-Xlog:heapdump`; > - tier1,tier2,hs-tier5-svc; > - all reg.tests that use heap dump. This pull request has now been integrated. Changeset: 0a24daec Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/0a24daecebd90eb46a813923bb2d5672514197ce Stats: 16 lines in 3 files changed: 12 ins; 0 del; 4 mod 8322043: HeapDumper should use parallel dump by default Reviewed-by: yyang, sspitsyn, dholmes - PR: https://git.openjdk.org/jdk/pull/18748
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly [v3]
On Tue, 30 Apr 2024 02:05:10 GMT, Serguei Spitsyn wrote: >> Looks like in JVMTI `current_thread` is more common (and `current` is >> usually used in runtime :) > > The plan is to unify this with the approach used by the Runtime team. Replaced all touched "current_thread" and "calling_thread" with "current" - PR Review Comment: https://git.openjdk.org/jdk/pull/18986#discussion_r1585718011
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly [v3]
> Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method > > Testing: tier1-6 Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: renamed current_thread tp current - Changes: - all: https://git.openjdk.org/jdk/pull/18986/files - new: https://git.openjdk.org/jdk/pull/18986/files/d5d614bc..46026322 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18986=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18986=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18986/head:pull/18986 PR: https://git.openjdk.org/jdk/pull/18986
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly [v2]
> Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method > > Testing: tier1-6 Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: renamed current_thread to current - Changes: - all: https://git.openjdk.org/jdk/pull/18986/files - new: https://git.openjdk.org/jdk/pull/18986/files/f472f669..d5d614bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18986=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18986=00-01 Stats: 131 lines in 2 files changed: 0 ins; 1 del; 130 mod Patch: https://git.openjdk.org/jdk/pull/18986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18986/head:pull/18986 PR: https://git.openjdk.org/jdk/pull/18986
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v3]
On Tue, 30 Apr 2024 01:56:13 GMT, Serguei Spitsyn wrote: >> This is a fix of the following JVMTI scalability issue. A closed benchmark >> with millions of virtual threads shows 3X-4X overhead when a JVMTI agent has >> been loaded. For instance, this is observable when an app is executed under >> control of the Oracle Studio `collect` utility. >> For performance analysis, experiments and numbers, please, see the comment >> below this description. >> >> The fix is to replace the global counter `_VTMS_transition_count` with the >> mark bit `_VTMS_transition_mark` in each `JavaThread`'. >> >> Testing: >> - Tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: correct comments related to VTMS transition counters Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18937#pullrequestreview-2032255537
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v6]
> The fix makes VM heap dumping parallel by default. > `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the fix > affects `HotSpotDiagnosticMXBean.dumpHeap()`, `-XX:+HeapDumpBeforeFullGC`, > `-XX:+HeapDumpAfterFullGC` and `-XX:+HeapDumpOnOutOfMemoryError`. > > Testing: > - manually tested different heap dump scenarios with `-Xlog:heapdump`; > - tier1,tier2,hs-tier5-svc; > - all reg.tests that use heap dump. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: removed space-only change in diagnosticCommand.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/18748/files - new: https://git.openjdk.org/jdk/pull/18748/files/641bedc5..eb38705d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18748=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18748=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18748.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18748/head:pull/18748 PR: https://git.openjdk.org/jdk/pull/18748
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly
On Fri, 26 Apr 2024 23:36:41 GMT, Chris Plummer wrote: >> Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method >> >> Testing: tier1-6 > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 1976: > >> 1974: oop thread_obj = nullptr; >> 1975: >> 1976: jvmtiError err = >> JvmtiEnvBase::get_threadOop_and_JavaThread(tlh.list(), target, current, >> _thread, _obj); > > I think a good cleanup would be to also replace `current` with > `current_thread`, although I'm not sure how common each are. I see 3 > `current` references in this webrev. Looks like in JVMTI `current_thread` is more common (and `current` is usually used in runtime :) - PR Review Comment: https://git.openjdk.org/jdk/pull/18986#discussion_r1581628063
RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly
Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method Testing: tier1-6 - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/18986/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18986=00 Issue: https://bugs.openjdk.org/browse/JDK-8330852 Stats: 42 lines in 3 files changed: 3 ins; 10 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/18986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18986/head:pull/18986 PR: https://git.openjdk.org/jdk/pull/18986
Re: RFR: 8330969: scalability issue with loaded JVMTI agent
On Wed, 24 Apr 2024 16:04:30 GMT, Serguei Spitsyn wrote:
> This is a fix of the following JVMTI scalability issue. A closed benchmark
> with millions of virtual threads shows 3X-4X overhead when a JVMTI agent has
> been loaded. For instance, this is observable when an app is executed under
> control of the Oracle Studio `collect` utility.
> For performance analysis, experiments and numbers, please, see the comment
> below this description.
>
> The fix is to replace the global counter `_VTMS_transition_count` with the
> mark bit `_VTMS_transition_mark` in each `JavaThread`'.
>
> Testing:
> - Tested with mach5 tiers 1-6
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1638:
> 1636: // Iterates over all JavaThread's, counts VTMS transitions and
> restores
> 1637: // jt->jvmti_thread_state() and jt->jvmti_vthread() for VTMS
> transition protocol.
> 1638: void count_transitions_and_correct_jvmti_thread_states() {
The method doesn't count anything anymore.
Rename it to `correct_jvmti_thread_states()`?
Comment needs to be updated too.
src/hotspot/share/prims/jvmtiThreadState.cpp line 501:
> 499: oop vt = JNIHandles::resolve_external_guard(vthread);
> 500: java_lang_Thread::set_is_in_VTMS_transition(vt, false);
> 501: assert(thread->VTMS_transition_mark(), "sanity ed_heck");
Suggestion:
assert(thread->VTMS_transition_mark(), "sanity check");
src/hotspot/share/runtime/javaThread.hpp line 668:
> 666: void toggle_is_disable_suspend() { _is_disable_suspend =
> !_is_disable_suspend; };
> 667:
> 668: bool VTMS_transition_mark(){ return
> Atomic::load(&_VTMS_transition_mark); }
Suggestion:
bool VTMS_transition_mark() const { return
Atomic::load(&_VTMS_transition_mark); }
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1580096556
PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1580101609
PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1580108674
Integrated: 8329113: Deprecate -XX:+UseNotificationThread
On Fri, 19 Apr 2024 01:16:46 GMT, Alex Menkov wrote: > The fix deprecates -XX:+UseNotificationThread VM option > > Testing: tier1-3,hs-tier5-svc This pull request has now been integrated. Changeset: 25551662 Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/2555166247230497453503318ccbf4dd4f047193 Stats: 3 lines in 3 files changed: 2 ins; 0 del; 1 mod 8329113: Deprecate -XX:+UseNotificationThread Reviewed-by: dcubed, dholmes, coleenp - PR: https://git.openjdk.org/jdk/pull/18852
Re: RFR: 8329113: Deprecate -XX:+UseNotificationThread [v2]
> The fix deprecates -XX:+UseNotificationThread VM option > > Testing: tier1-3,hs-tier5-svc Alex Menkov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge branch 'master' into deprecate_notif_thread - fix - Changes: https://git.openjdk.org/jdk/pull/18852/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18852=01 Stats: 3 lines in 3 files changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18852.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18852/head:pull/18852 PR: https://git.openjdk.org/jdk/pull/18852
Re: RFR: 8329113: Deprecate -XX:+UseNotificationThread
On Fri, 19 Apr 2024 16:18:56 GMT, Daniel D. Daugherty wrote: > Have you looked for any tests that are using this option? No tests use the option - PR Comment: https://git.openjdk.org/jdk/pull/18852#issuecomment-2067059488
RFR: 8329113: Deprecate -XX:+UseNotificationThread
The fix deprecates -XX:+UseNotificationThread VM option Testing: tier1-3 - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/18852/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18852=00 Issue: https://bugs.openjdk.org/browse/JDK-8329113 Stats: 3 lines in 3 files changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18852.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18852/head:pull/18852 PR: https://git.openjdk.org/jdk/pull/18852
RFR: 8320215: HeapDumper can use DumpWriter buffer during merge
The fix updates HeapMerger to use writer buffer (no need to copy memory, also writer buffer is 1MB instead of 4KB). Additionally fixed small issue in FileWriter (looks like `ssize_t` instead of `size_t` is a typo, the argument should be unsigned) Testing: all HeapDump-related tests on Oracle supported platforms - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/18850/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18850=00 Issue: https://bugs.openjdk.org/browse/JDK-8320215 Stats: 22 lines in 3 files changed: 9 ins; 4 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/18850.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18850/head:pull/18850 PR: https://git.openjdk.org/jdk/pull/18850
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v5]
> The fix makes VM heap dumping parallel by default. > `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the fix > affects `HotSpotDiagnosticMXBean.dumpHeap()`, `-XX:+HeapDumpBeforeFullGC`, > `-XX:+HeapDumpAfterFullGC` and `-XX:+HeapDumpOnOutOfMemoryError`. > > Testing: > - manually tested different heap dump scenarios with `-Xlog:heapdump`; > - tier1,tier2,hs-tier5-svc; > - all reg.tests that use heap dump. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: removed unneeded cast - Changes: - all: https://git.openjdk.org/jdk/pull/18748/files - new: https://git.openjdk.org/jdk/pull/18748/files/482d9ffe..641bedc5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18748=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18748=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18748.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18748/head:pull/18748 PR: https://git.openjdk.org/jdk/pull/18748
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v4]
> The fix makes VM heap dumping parallel by default. > `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the fix > affects `HotSpotDiagnosticMXBean.dumpHeap()`, `-XX:+HeapDumpBeforeFullGC`, > `-XX:+HeapDumpAfterFullGC` and `-XX:+HeapDumpOnOutOfMemoryError`. > > Testing: > - manually tested different heap dump scenarios with `-Xlog:heapdump`; > - tier1,tier2,hs-tier5-svc; > - all reg.tests that use heap dump. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: feedback - Changes: - all: https://git.openjdk.org/jdk/pull/18748/files - new: https://git.openjdk.org/jdk/pull/18748/files/f6db604f..482d9ffe Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18748=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18748=02-03 Stats: 18 lines in 3 files changed: 0 ins; 3 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/18748.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18748/head:pull/18748 PR: https://git.openjdk.org/jdk/pull/18748
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v3]
On Tue, 16 Apr 2024 09:08:20 GMT, David Holmes wrote: >> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> check free_memory for OOME > > src/hotspot/share/services/heapDumper.hpp line 63: > >> 61: // additional info is written to out if not null. >> 62: // compression >= 0 creates a gzipped file with the given compression >> level. >> 63: // parallel_thread_num >= 0 indicates thread numbers of parallel >> object dump, -1 means "auto select". > > I don't understand why you need to add `-1` to mean "auto-select" instead of > just setting the default parameter to be `default_num_of_dump_threads()`? I think it makes the code more flexible - it allows to distinguish between "use default value" and "I don't care" cases. - PR Review Comment: https://git.openjdk.org/jdk/pull/18748#discussion_r1567864860
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v3]
> The fix makes VM heap dumping parallel by default. > `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the fix > affects `HotSpotDiagnosticMXBean.dumpHeap()`, `-XX:+HeapDumpBeforeFullGC`, > `-XX:+HeapDumpAfterFullGC` and `-XX:+HeapDumpOnOutOfMemoryError`. > > Testing: > - manually tested different heap dump scenarios with `-Xlog:heapdump`; > - tier1,tier2,hs-tier5-svc; > - all reg.tests that use heap dump. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: check free_memory for OOME - Changes: - all: https://git.openjdk.org/jdk/pull/18748/files - new: https://git.openjdk.org/jdk/pull/18748/files/7ae4806e..f6db604f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18748=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18748=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18748.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18748/head:pull/18748 PR: https://git.openjdk.org/jdk/pull/18748
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v2]
> The fix makes VM heap dumping parallel by default. > `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the fix > affects `HotSpotDiagnosticMXBean.dumpHeap()`, `-XX:+HeapDumpBeforeFullGC`, > `-XX:+HeapDumpAfterFullGC` and `-XX:+HeapDumpOnOutOfMemoryError`. > > Testing: > - manually tested different heap dump scenarios with `-Xlog:heapdump`; > - tier1,tier2,hs-tier5-svc; > - all reg.tests that use heap dump. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: check free_memory for OOME - Changes: - all: https://git.openjdk.org/jdk/pull/18748/files - new: https://git.openjdk.org/jdk/pull/18748/files/8a66572e..7ae4806e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18748=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18748=00-01 Stats: 10 lines in 1 file changed: 10 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18748.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18748/head:pull/18748 PR: https://git.openjdk.org/jdk/pull/18748
Integrated: 8240343: JDI stopListening/stoplis001 "FAILED: listening is successfully stopped without starting listening"
On Wed, 10 Apr 2024 00:28:01 GMT, Alex Menkov wrote: > The test starts listening on dynamic port and calls stopListening with > incorrect (cArgs1) and correct (cArgs2) argument maps. > Incorrect map is created by finding "free" port (`(new > ServerSocket(0)).getLocalPort()`) > The test fails if the same port is selected later when the listening starts. > To avoid this free port should be picked after listening on dynamic port > started. > > Additionally removed unnecessary `exclusiveAccess.dirs=.` test property and > incorrect comment in the description. > The test uses `SocketListen` connectors, the coment is about > `SharedMemoryListen` connector. > > Testing: run the test on all Oracle-supported platforms 100 times This pull request has now been integrated. Changeset: 28b20195 Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/28b201955907e145f208d756b607ab545a83b2d3 Stats: 34 lines in 3 files changed: 6 ins; 28 del; 0 mod 8240343: JDI stopListening/stoplis001 "FAILED: listening is successfully stopped without starting listening" Reviewed-by: cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/18705
Re: RFR: 8240343: JDI stopListening/stoplis001 "FAILED: listening is successfully stopped without starting listening" [v2]
On Fri, 12 Apr 2024 18:05:59 GMT, Chris Plummer wrote: > You might want to consider adding the following diff. It will help in case > this issue (or similar) reproduces again: > makes sense. added. - PR Comment: https://git.openjdk.org/jdk/pull/18705#issuecomment-2052542650
Re: RFR: 8240343: JDI stopListening/stoplis001 "FAILED: listening is successfully stopped without starting listening" [v3]
> The test starts listening on dynamic port and calls stopListening with > incorrect (cArgs1) and correct (cArgs2) argument maps. > Incorrect map is created by finding "free" port (`(new > ServerSocket(0)).getLocalPort()`) > The test fails if the same port is selected later when the listening starts. > To avoid this free port should be picked after listening on dynamic port > started. > > Additionally removed unnecessary `exclusiveAccess.dirs=.` test property and > incorrect comment in the description. > The test uses `SocketListen` connectors, the coment is about > `SharedMemoryListen` connector. > > Testing: run the test on all Oracle-supported platforms 100 times Alex Menkov has updated the pull request incrementally with two additional commits since the last revision: - Merge branch 'stopListening' of github.com:alexmenkov/jdk into stopListening - arg map logging - Changes: - all: https://git.openjdk.org/jdk/pull/18705/files - new: https://git.openjdk.org/jdk/pull/18705/files/3b0d5540..a04363d6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18705=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18705=01-02 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18705.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18705/head:pull/18705 PR: https://git.openjdk.org/jdk/pull/18705
Re: RFR: 8322043: HeapDumper should use parallel dump by default
On Fri, 12 Apr 2024 06:07:31 GMT, Thomas Stuefe wrote: > I am curious: what is the memory overhead for parallel mode, and (I am not > familiar with the logic) how many threads are involved? Is the number of > thread bounded? > > I ask because, especially for the OnOOM handling, we may already be at a > limit memory-wise. Starting to swap will probably be worse than running > single-threaded. Good question. It think it's several MB per each additional thread (1MB output buffer, DumperClassCacheTable - 1031 elements max, element size depends on class field numbers, if HeapDumpGzipLevel is set, some buffers for gzip compressors) Number of threads by default is min of `os::initial_active_processor_count() * 3 / 8` and number of GC workers. - PR Comment: https://git.openjdk.org/jdk/pull/18748#issuecomment-2052312786
RFR: 8322043: HeapDumper should use parallel dump by default
The fix makes VM heap dumping parallel by default. `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the fix affects `HotSpotDiagnosticMXBean.dumpHeap()`, `-XX:+HeapDumpBeforeFullGC`, `-XX:+HeapDumpAfterFullGC` and `-XX:+HeapDumpOnOutOfMemoryError`. Testing: - manually tested different heap dump scenarios with `-Xlog:heapdump`; - tier1,tier2,hs-tier5-svc; - all reg.tests that use heap dump. - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/18748/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18748=00 Issue: https://bugs.openjdk.org/browse/JDK-8322043 Stats: 15 lines in 4 files changed: 5 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/18748.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18748/head:pull/18748 PR: https://git.openjdk.org/jdk/pull/18748
Re: RFR: JDK-8240343: JDI stopListening/stoplis001 "FAILED: listening is successfully stopped without starting listening" [v2]
On Wed, 10 Apr 2024 04:23:30 GMT, Chris Plummer wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update
>> test/hotspot/jtreg/vmTestbase/nsk/jdi/ListeningConnector/stopListening/stoplis001.java
>>
>> Co-authored-by: Chris Plummer
>
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ListeningConnector/stopListening/stoplis001.java
> line 98:
>
>> 96: log.display("TEST: start listening the address " + addr);
>> 97:
>> 98: // argHandler.getTransportPort() returns a free port (different
>> from the listening port in cArgs2)
>
> Suggestion:
>
> // argHandler.getTransportPort() returns a free port (different from
> the port allocated by startListen(cArgs2))
done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18705#discussion_r1559838917
Re: RFR: JDK-8240343: JDI stopListening/stoplis001 "FAILED: listening is successfully stopped without starting listening" [v2]
> The test starts listening on dynamic port and calls stopListening with > incorrect (cArgs1) and correct (cArgs2) argument maps. > Incorrect map is created by finding "free" port (`(new > ServerSocket(0)).getLocalPort()`) > The test fails if the same port is selected later when the listening starts. > To avoid this free port should be picked after listening on dynamic port > started. > > Additionally removed unnecessary `exclusiveAccess.dirs=.` test property and > incorrect comment in the description. > The test uses `SocketListen` connectors, the coment is about > `SharedMemoryListen` connector. > > Testing: run the test on all Oracle-supported platforms 100 times Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: Update test/hotspot/jtreg/vmTestbase/nsk/jdi/ListeningConnector/stopListening/stoplis001.java Co-authored-by: Chris Plummer - Changes: - all: https://git.openjdk.org/jdk/pull/18705/files - new: https://git.openjdk.org/jdk/pull/18705/files/3f9bc6b7..3b0d5540 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18705=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18705=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18705.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18705/head:pull/18705 PR: https://git.openjdk.org/jdk/pull/18705
RFR: JDK-8240343: JDI stopListening/stoplis001 "FAILED: listening is successfully stopped without starting listening"
The test starts listening on dynamic port and calls stopListening with incorrect (cArgs1) and correct (cArgs2) argument maps. Incorrect map is created by finding "free" port (`(new ServerSocket(0)).getLocalPort()`) The test fails if the same port is selected later when the listening starts. To avoid this free port should be picked after listening on dynamic port started. Additionally removed unnecessary `exclusiveAccess.dirs=.` test property and incorrect comment in the description. The test uses `SocketListen` connectors, the coment is about `SharedMemoryListen` connector. Testing: run the test on all Oracle-supported platforms 100 times - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/18705/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18705=00 Issue: https://bugs.openjdk.org/browse/JDK-8240343 Stats: 31 lines in 3 files changed: 3 ins; 28 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18705.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18705/head:pull/18705 PR: https://git.openjdk.org/jdk/pull/18705
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v10]
On Wed, 27 Mar 2024 13:44:42 GMT, Matthias Baesken wrote: >> Currently jcmd command GC.heap_dump only works with an additionally provided >> file name. >> Syntax : GC.heap_dump [options] >> >> In case the JVM has the XX - flag HeapDumpPath set, we should support an >> additional mode where the is optional. >> In case the filename is NOT set, we take the HeapDumpPath (file or >> directory); >> >> new syntax : >> GC.heap_dump [options] .. has precedence over second option >> GC.heap_dump [options] …in case -XX: HeapDumpPath=p is set >> >> This would be a simplification e.g. for support cases where a filename or >> directory is set at JVM startup with -XX: HeapDumpPath=p and writing to the >> path is intended/recommended for usage also in the jcmd case. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust java.1 man page I don't have strong opinion if it's good to have default file path for `jcmd GC.heap_dump`, just some thoughts. We have several ways to heap dump: 1. `-XX:+HeapDumpOnOutOfMemoryError` 2. `-XX:+HeapDumpBeforeFullGC`, `-XX:+HeapDumpAfterFullGC` in the case `HeapDumpPath` and `HeapDumpGzipLevel` are used (this is not mentioned in the options' description, need to fix it) 3. `jcmd GC.heap_dump` 4. `jmap -dump` it uses "dumpheap" Attach operation, implemented by AttachListener directly 5. `HotSpotDiagnosticMXBean.dumpHeap()`; Current patch looks inconsistent: If HeapDumpPath is used as default, HeapDumpGzipLevel should be used as default too (note that default path has different extension depending on HeapDumpGzipLevel); If (3) has defaults, why (4) and (maybe) (5) don't have the same defaults? - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2040728112
Integrated: JDK-8322042: HeapDumper should perform merge on the current thread instead of VMThread
On Tue, 2 Apr 2024 00:40:37 GMT, Alex Menkov wrote: > The fix updated HeapDumper to always perform merge on the current thread. > > Testing: tier1-5, all HeapDump-related tests > Covered heap dumping scenarios: > - `jcmd GC.heap_dump` command; > - `HotSpotDiagnosticMXBean.dumpHeap()`; > - `HeapDumpBeforeFullGC`, `HeapDumpAfterFullGC` VM options; > - `HeapDumpOnOutOfMemoryError` VM option. This pull request has now been integrated. Changeset: 12ad09a9 Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/12ad09a966030e7ed0900e205df4b412cf0b6a32 Stats: 26 lines in 2 files changed: 0 ins; 22 del; 4 mod 8322042: HeapDumper should perform merge on the current thread instead of VMThread Reviewed-by: sspitsyn, kevinw - PR: https://git.openjdk.org/jdk/pull/18571
Re: RFR: JDK-8322042: HeapDumper should perform merge on the current thread instead of VMThread [v2]
On Wed, 3 Apr 2024 10:09:20 GMT, Kevin Walls wrote: > Are we saying there is never any need to perform the merge in a VM Operation? > Originally (JDK-8306441) it's either done in the attach thread, or a VM > operation if we are in another thread. But maybe that was just being cautious. Correct. Originally it was implemented for AttachListener thread only for safety (and heap dump using jcmd was point of interest). I didn't find issues with concurrent dumping (actually with concurrent merging, dumping itself is performed in VM_HeapDumper at safepoint). - PR Comment: https://git.openjdk.org/jdk/pull/18571#issuecomment-2035530652
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v2]
On Wed, 3 Apr 2024 13:25:36 GMT, Coleen Phillimore wrote: >> This change simplifies the code that grows the jmethodID cache in >> InstanceKlass. Instead of lazily, when there's a rare request for a >> jmethodID for an obsolete method, the jmethodID cache is grown during the >> RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily >> allocated when there's a jmethodID allocated, so not every InstanceKlass has >> a cache, but the growth now only happens in a safepoint. This code will >> become racy with the potential change for deallocating jmethodIDs. >> >> Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in >> case they're not in tier1-4). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Refactoring suggested by Serguei. Still looks good - Marked as reviewed by amenkov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18549#pullrequestreview-1977650540
Integrated: JDK-8328137: PreserveAllAnnotations can cause failure of class retransformation
On Thu, 28 Mar 2024 22:12:49 GMT, Alex Menkov wrote: > PreserveAllAnnotations option causes class file parser to preserve > RuntimeInvisibleAnnotations so VM considers them as RuntimeVisibleAnnotations. > For class retransformation JvmtiClassFileReconstituter restores all > annotations as RuntimeVisibleAnnotations attributes. > This can cause problem is the class contains only > RuntimeInvisibleAnnotations, so corresponding RuntimeVisibleAnnotations > attribute name is not present in the class constant pool. > > Correct solution would be to store additional information about > RuntimeInvisibleAnnotations and restore them exactly as they were in the > original class (this should be done for all annotations: > RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations for class, fields > and records, > RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations/RuntimeInvisibleParameterAnnotations > for methods; need to ensure the information is correctly updated during > class redefinition & retransformation). > > I think it doesn't make sense to add all the complexity for almost no value > (I doubt anyone uses PreserveAllAnnotations, the flag looks like > experimental, we don't have any tests for it). > > The suggested fix adds workaround for this corner case - if "visible" > attribute name is not in the CP, the annotations are restored with > "invisible" attribute name. > > Testing: > - tier1,tier2,hs-tier5-svc > - all java/lang/instrument tests; > - all RedefineClasses/RetransformClasses tests This pull request has now been integrated. Changeset: f88f31dc Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/f88f31dcbf80e9a4cd3ba9d34be8b88128af97c6 Stats: 33 lines in 3 files changed: 22 ins; 0 del; 11 mod 8328137: PreserveAllAnnotations can cause failure of class retransformation Reviewed-by: coleenp, sspitsyn - PR: https://git.openjdk.org/jdk/pull/18540
Re: RFR: JDK-8328137: PreserveAllAnnotations can cause failure of class retransformation
On Tue, 2 Apr 2024 19:13:15 GMT, Coleen Phillimore wrote: > At one point long ago, I was trying to understand why we have > PreserveAllAnnotations and couldn't come up with a reason. For a class file > reconstitutor, restoring the invisible annotations to the classfile and then > feeding it back to the JVM should have no effect, since the VM doesn't do > anything with these annotations. > > I see now why you get the original assert. I think this looks like a > reasonable workaround for this problem. > > I wonder if we can deprecate PreserveAllAnnotations. Wonder what it's for? It > would need a CSR because it's a product flag. I also was not able to identify purpose of the flag. I found couple PRs from 2021 about the flag: #4245 and #4280 with a description of possible usecase for it, but it does not look as a good reason to me. I'll create a CR to deprecate the flag. - PR Comment: https://git.openjdk.org/jdk/pull/18540#issuecomment-2033319487
Re: RFR: JDK-8322042: HeapDumper should perform merge on the current thread instead of VMThread [v2]
On Tue, 2 Apr 2024 20:44:13 GMT, Serguei Spitsyn wrote: >> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> comment > > src/hotspot/share/services/heapDumper.cpp line 2648: > >> 2646: >> 2647: DumpMerger merger(path, , dumper.dump_seq()); >> 2648: // perform heapdump file merge operation in the current thread >> prevents us > > Nit: Better to start with a capital letter. Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/18571#discussion_r1548639189
Re: RFR: JDK-8322042: HeapDumper should perform merge on the current thread instead of VMThread [v2]
On Tue, 2 Apr 2024 02:04:17 GMT, Yi Yang wrote: > * jcmd GC.heap_dump command; `AttachListenerThread` > > * HotSpotDiagnosticMXBean.dumpHeap(); `JavaThread` > > * HeapDumpBeforeFullGC, HeapDumpAfterFullGC VM options; `VMThread` > > * HeapDumpOnOutOfMemoryError VM option. `VMThread` > Mabye we can always use AttachListenerThread(via Handshake) or new > virtual thread? AttachListenerThread may not exist (it's created on demand). Heap dump for HeapDumpOnOutOfMemoryError is performed on the current java thread. I'm not sure I follow the proposal. What if the profit in using separate thread for merging? - PR Comment: https://git.openjdk.org/jdk/pull/18571#issuecomment-2033101158
Re: RFR: JDK-8322042: HeapDumper should perform merge on the current thread instead of VMThread [v2]
> The fix updated HeapDumper to always perform merge on the current thread. > > Testing: tier1-5, all HeapDump-related tests > Covered heap dumping scenarios: > - `jcmd GC.heap_dump` command; > - `HotSpotDiagnosticMXBean.dumpHeap()`; > - `HeapDumpBeforeFullGC`, `HeapDumpAfterFullGC` VM options; > - `HeapDumpOnOutOfMemoryError` VM option. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: comment - Changes: - all: https://git.openjdk.org/jdk/pull/18571/files - new: https://git.openjdk.org/jdk/pull/18571/files/e373d793..2eb802f7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18571=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18571=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18571.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18571/head:pull/18571 PR: https://git.openjdk.org/jdk/pull/18571
Re: RFR: JDK-8322042: HeapDumper should perform merge on the current thread instead of VMThread
On Tue, 2 Apr 2024 00:40:37 GMT, Alex Menkov wrote: > The fix updated HeapDumper to always perform merge on the current thread. > > Testing: tier1-5, all HeapDump-related tests > Covered heap dumping scenarios: > - `jcmd GC.heap_dump` command; > - `HotSpotDiagnosticMXBean.dumpHeap()`; > - `HeapDumpBeforeFullGC`, `HeapDumpAfterFullGC` VM options; > - `HeapDumpOnOutOfMemoryError` VM option. added serviceability label - PR Comment: https://git.openjdk.org/jdk/pull/18571#issuecomment-2030892178
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass
On Fri, 29 Mar 2024 15:25:48 GMT, Coleen Phillimore wrote: > This change simplifies the code that grows the jmethodID cache in > InstanceKlass. Instead of lazily, when there's a rare request for a > jmethodID for an obsolete method, the jmethodID cache is grown during the > RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily > allocated when there's a jmethodID allocated, so not every InstanceKlass has > a cache, but the growth now only happens in a safepoint. This code will > become racy with the potential change for deallocating jmethodIDs. > > Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in case > they're not in tier1-4). Looks like good simplification - Marked as reviewed by amenkov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18549#pullrequestreview-1972336247
Re: RFR: JDK-8328137: PreserveAllAnnotations can cause failure of class retransformation
On Fri, 29 Mar 2024 07:08:58 GMT, Serguei Spitsyn wrote:
> > Correct solution would be to store additional information about
> > RuntimeInvisibleAnnotations and restore them exactly as they were in the
> > original class (this should be done for all annotations:
> > RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations for class,
> > fields and records,
> > RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations/RuntimeInvisibleParameterAnnotations
> > for methods; need to ensure the information is correctly updated during
> > class redefinition & retransformation).
> > I think it doesn't make sense to add all the complexity for almost no value
> > (I doubt anyone uses PreserveAllAnnotations, the flag looks like
> > experimental, we don't have any tests for it).
>
> This solution looks okay in general but still is a little bit confusing. It
> feels like saving just one bit would not add much complexity but would even
> simplify the implementation as it will become straight-forward. At least,
> there would be no need in this additional function with the
> `fallback_attr_name` parameter.
It's a bit more complicated than storing 1 bit.
Lets consider ClassFileParser::parse_classfile_record_attribute as an example.
There are 2 type of annotations for records: RuntimeVisibleAnnotations and
RuntimeVisibleTypeAnnotations.
For each type if PreserveAllAnnotations is set ClassFileParser additionally
handles invisible annotations
(RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations).
Then it "assembles" annotations (concatenates visible and invisible annotations
into single array).
To restore original annotations we need to keep number of visible annotations
in the annotation array (so jvmtiClassReconstituter writes part of the
annotation array as visible, and the rest as invisible).
And we need to store this number for each annotation type (Annotations and
TypeAnnotations for records, Annotations and TypeAnnotations for class,
Annotations and TypeAnnotations for fields,
Annotations/TypeAnnotations/ParameterAnnotations for methods).
I considered another way to implement the workaround - add method
`bool is_symbol_in_cp(const char* symbol)`
But then each writing would become very verbose and more confusing (needs some
explanation in each place):
if (is_symbol_in_cp("RuntimeVisibleTypeAnnotations")) {
write_annotations_attribute("RuntimeVisibleTypeAnnotations",
component->type_annotations());
} else {
write_annotations_attribute("RuntimeInvisibleTypeAnnotations",
component->type_annotations());
}
-
PR Comment: https://git.openjdk.org/jdk/pull/18540#issuecomment-2027647051
RFR: JDK-8328137: PreserveAllAnnotations can cause failure of class retransformation
PreserveAllAnnotations option causes class file parser to preserve RuntimeInvisibleAnnotations so VM considers them as RuntimeVisibleAnnotations. For class retransformation JvmtiClassFileReconstituter restores all annotations as RuntimeVisibleAnnotations attributes. This can cause problem is the class contains only RuntimeInvisibleAnnotations, so corresponding RuntimeVisibleAnnotations attribute name is not present in the class constant pool. Correct solution would be to store additional information about RuntimeInvisibleAnnotations and restore them exactly as they were in the original class (this should be done for all annotations: RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations for class, fields and records, RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations/RuntimeInvisibleParameterAnnotations for methods; need to ensure the information is correctly updated during class redefinition & retransformation). I think it doesn't make sense to add all the complexity for almost no value (I doubt anyone uses PreserveAllAnnotations, the flag looks like experimental, we don't have any tests for it). The suggested fix adds workaround for this corner case - if "visible" attribute name is not in the CP, the annotations are restored with "invisible" attribute name. Testing: - tier1,tier2,hs-tier5-svc - all java/lang/instrument tests; - all RedefineClasses/RetransformClasses tests - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/18540/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18540=00 Issue: https://bugs.openjdk.org/browse/JDK-8328137 Stats: 33 lines in 3 files changed: 22 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/18540.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18540/head:pull/18540 PR: https://git.openjdk.org/jdk/pull/18540
Integrated: JDK-8315575: Retransform of record class with record component annotation fails with CFE
On Fri, 8 Mar 2024 02:54:49 GMT, Alex Menkov wrote: > RecordComponent class has _attributes_count field. > The only user of the field is JvmtiClassFileReconstituter. Incorrect value of > the field causes producing incorrect data for Record attribute. > Parsing Record attribute ClassFileParser skips unknown attributes and may > skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations. > Also annotations can be changed (added/removed) by class redefinition. > The fix removes attributes_count from RecordComponent; > JvmtiClassFileReconstituter calculates correct attributes_count generating > class bytes. > > Testing: > - tier1,tier2,hs-tier5-svc; > - redefineClasses/retransformClasses tests: >- test/jdk/java/lang/instrument >- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses This pull request has now been integrated. Changeset: 8fc9097b Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19 Stats: 208 lines in 5 files changed: 190 ins; 10 del; 8 mod 8315575: Retransform of record class with record component annotation fails with CFE Reviewed-by: sspitsyn, coleenp - PR: https://git.openjdk.org/jdk/pull/18161
Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v7]
On Tue, 26 Mar 2024 19:10:11 GMT, Coleen Phillimore wrote: >> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> added comment > > test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 30: > >> 28: * >> 29: * @library /test/lib >> 30: * @run shell MakeJAR.sh retransformAgent > > Does the presence of attributes mean you can't use the simpler > RedefineClassHelper that the tests in > test/hotspot/jtreg/serviceability/jvmti/RedefineClasses use? RedefineClassHelper supports only class redefinition, the test needs class retransformation. - PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1540044735
Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v7]
On Thu, 21 Mar 2024 18:10:49 GMT, Alex Menkov wrote: >> RecordComponent class has _attributes_count field. >> The only user of the field is JvmtiClassFileReconstituter. Incorrect value >> of the field causes producing incorrect data for Record attribute. >> Parsing Record attribute ClassFileParser skips unknown attributes and may >> skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations. >> Also annotations can be changed (added/removed) by class redefinition. >> The fix removes attributes_count from RecordComponent; >> JvmtiClassFileReconstituter calculates correct attributes_count generating >> class bytes. >> >> Testing: >> - tier1,tier2,hs-tier5-svc; >> - redefineClasses/retransformClasses tests: >>- test/jdk/java/lang/instrument >>- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses >>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses >>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > added comment Can I get 2nd review plase - PR Comment: https://git.openjdk.org/jdk/pull/18161#issuecomment-2021144086
Re: RFR: JDK-8328303: 3 JDI tests timed out with UT enabled [v3]
On Fri, 22 Mar 2024 21:29:03 GMT, Alex Menkov wrote: >> The change fixes 3 nsk JDI tests. >> Root cause in all 3 tests is the same - the tests requests JDI event with >> SUSPEND_ALL policy, but event handler thread stops handle incoming event and >> this causes debuggee to hang (suspended by JDI event). >> >> All 3 tests are updated to exit event handler thread after getting >> VMDeathEvent or VMDisconnectEvent (and resume debuggee after any other >> events). >> ClassPrepareEvent tests need to wait some time to allow handle all expected >> events before terminate the debuggee. The logic was implemented by using >> CountDownLatch. >> >> All tests are passed with "--test-repeat 20" > > Alex Menkov has updated the pull request incrementally with two additional > commits since the last revision: > > - updated comments > - eventsReceived -> allEventsReceived Last update was cosmetic, so I'm going to integrate the fix to reduce noise in CI - PR Comment: https://git.openjdk.org/jdk/pull/18442#issuecomment-2016125851
Integrated: JDK-8328303: 3 JDI tests timed out with UT enabled
On Thu, 21 Mar 2024 22:25:23 GMT, Alex Menkov wrote: > The change fixes 3 nsk JDI tests. > Root cause in all 3 tests is the same - the tests requests JDI event with > SUSPEND_ALL policy, but event handler thread stops handle incoming event and > this causes debuggee to hang (suspended by JDI event). > > All 3 tests are updated to exit event handler thread after getting > VMDeathEvent or VMDisconnectEvent (and resume debuggee after any other > events). > ClassPrepareEvent tests need to wait some time to allow handle all expected > events before terminate the debuggee. The logic was implemented by using > CountDownLatch. > > All tests are passed with "--test-repeat 20" This pull request has now been integrated. Changeset: bc739639 Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/bc73963974a824d77d54b8b0edbf8f05262b721c Stats: 97 lines in 3 files changed: 37 ins; 22 del; 38 mod 8328303: 3 JDI tests timed out with UT enabled Reviewed-by: cjplummer, sspitsyn, dcubed - PR: https://git.openjdk.org/jdk/pull/18442
Re: RFR: JDK-8328303: 3 JDI tests timed out with UT enabled [v2]
On Fri, 22 Mar 2024 20:39:47 GMT, Serguei Spitsyn wrote: > One more question. Why these tests are failed with the UT enabled? Or maybe > this was wrong assumption? UT can add additional events (ClassPrepareEvent, ThreadDeathEvent). As the event are requested with SUSPEND_ALL policy, debuggee VM needs to be resumed. But the tests didn't expect "extra" events from other thread, event handler thread can exit before all events are handled, and tests fail with timeout waiting while debuggee exits. - PR Comment: https://git.openjdk.org/jdk/pull/18442#issuecomment-2015967581
Re: RFR: JDK-8328303: 3 JDI tests timed out with UT enabled [v3]
On Fri, 22 Mar 2024 04:09:28 GMT, Chris Plummer wrote:
>> Alex Menkov has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - updated comments
>> - eventsReceived -> allEventsReceived
>
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/referenceType/refType001.java
> line 276:
>
>> 274: try {
>> 275: if (!eventsReceivedLatch.await(eventTimeout,
>> TimeUnit.MILLISECONDS)) {
>> 276: log.complain("FAILURE 20: Timeout for waiting event
>> was exceeded");
>
> Should it be "Timeout waiting for all events was exceeded" or maybe "Timeout
> while waiting for all events"? Same thing for thread001.java.
Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536240012
Re: RFR: JDK-8328303: 3 JDI tests timed out with UT enabled [v2]
On Fri, 22 Mar 2024 20:27:31 GMT, Daniel D. Daugherty
wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback
>
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/referenceType/refType001.java
> line 117:
>
>> 115: boolean isConnected = true;
>> 116: boolean eventsReceived = false;
>> 117: // handle events until debugee is disconnected
>
> Nit typo: s/debugee/debuggee/
>
> But a lot of the NSK tests have this typo...
Fixed.
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/referenceType/refType001.java
> line 207:
>
>> 205: eventsReceived = true;
>> 206: for (int i = 0; i <
>> checkedTypes.length; i++) {
>> 207: if
>> (checkedTypes[i][2] == "0") {
>
> This if-statement could use a comment to explain the logic.
Done.
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/thread/thread001.java
> line 138:
>
>> 136: boolean isConnected = true;
>> 137: boolean eventsReceived = false;
>> 138: // handle events until debugee is disconnected
>
> Nit typo: s/debugee/debuggee/
>
> But a lot of the NSK tests have this typo...
Fixed.
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/thread/thread001.java
> line 233:
>
>> 231: }
>> 232:
>> 233: // Check that all expected
>> ClassPrepareEvent are received
>
> nit typo: s/ClassPrepareEvent/ClassPrepareEvent(s)/
>
> nit typo: Since this comment starts with a capital, it should have a period
> at the end.
Fixed.
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/thread/thread001.java
> line 237:
>
>> 235: eventsReceived = true;
>> 236: for (int i = 0; i <
>> checkedThreads.length; i++) {
>> 237: if
>> (checkedThreads[i][2] == "0") {
>
> This if-statement could use a comment to explain the logic.
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536241165
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536241466
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536241591
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536241959
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536242129
Re: RFR: JDK-8328303: 3 JDI tests timed out with UT enabled [v2]
On Fri, 22 Mar 2024 20:45:51 GMT, Alex Menkov wrote: >> test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/referenceType/refType001.java >> line 116: >> >>> 114: >>> 115: boolean isConnected = true; >>> 116: boolean eventsReceived = false; >> >> Nit: It'd better to rename this local to `allEventsReceived`. > > I tried to make minimal changes in the tests, so kept variable names to avoid > unnecessary changes Looks like all lines with the variable are already touched, I'll change the name - PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536212976
Re: RFR: JDK-8328303: 3 JDI tests timed out with UT enabled [v2]
On Fri, 22 Mar 2024 20:31:01 GMT, Serguei Spitsyn wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback
>
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/referenceType/refType001.java
> line 116:
>
>> 114:
>> 115: boolean isConnected = true;
>> 116: boolean eventsReceived = false;
>
> Nit: It'd better to rename this local to `allEventsReceived`.
I tried to make minimal changes in the tests, so kept variable names to avoid
unnecessary changes
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/referenceType/refType001.java
> line 134:
>
>> 132: while (eventIterator.hasNext()) {
>> 133: Event event = eventIterator.nextEvent();
>> 134: //log.display("\nEvent received:\n " +
>> event);
>
> Nit: Would it make sense to uncomment this log message? How many events are
> normally printed?
The only event received are ClassPrepareEvent (requested by test) and
VMDeathEvent (when debuggee exits)
Both types are logged below
> test/hotspot/jtreg/vmTestbase/nsk/jdi/ClassPrepareEvent/referenceType/refType001.java
> line 277:
>
>> 275: if (!eventsReceivedLatch.await(eventTimeout,
>> TimeUnit.MILLISECONDS)) {
>> 276: log.complain("FAILURE 20: Timeout waiting for all
>> events was exceeded");
>> 277: testFailed = true;
>
> As I understand the call to `eventHandler.interrupt()` is not needed now
> because now the `eventHandler` is expected to finish by itself when one of
> the events is received: `VMDeathEvent` or `VMDisconnectEvent`.
right. eventHandler exits when debuggee exits
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536200016
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536205663
PR Review Comment: https://git.openjdk.org/jdk/pull/18442#discussion_r1536207978
Re: RFR: JDK-8328303: 3 JDI tests timed out with UT enabled [v3]
> The change fixes 3 nsk JDI tests. > Root cause in all 3 tests is the same - the tests requests JDI event with > SUSPEND_ALL policy, but event handler thread stops handle incoming event and > this causes debuggee to hang (suspended by JDI event). > > All 3 tests are updated to exit event handler thread after getting > VMDeathEvent or VMDisconnectEvent (and resume debuggee after any other > events). > ClassPrepareEvent tests need to wait some time to allow handle all expected > events before terminate the debuggee. The logic was implemented by using > CountDownLatch. > > All tests are passed with "--test-repeat 20" Alex Menkov has updated the pull request incrementally with two additional commits since the last revision: - updated comments - eventsReceived -> allEventsReceived - Changes: - all: https://git.openjdk.org/jdk/pull/18442/files - new: https://git.openjdk.org/jdk/pull/18442/files/a30e3b70..53e4c460 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18442=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18442=01-02 Stats: 19 lines in 2 files changed: 5 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/18442.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18442/head:pull/18442 PR: https://git.openjdk.org/jdk/pull/18442
Re: RFR: JDK-8328303: 3 JDI tests timed out with UT enabled [v2]
> The change fixes 3 nsk JDI tests. > Root cause in all 3 tests is the same - the tests requests JDI event with > SUSPEND_ALL policy, but event handler thread stops handle incoming event and > this causes debuggee to hang (suspended by JDI event). > > All 3 tests are updated to exit event handler thread after getting > VMDeathEvent or VMDisconnectEvent (and resume debuggee after any other > events). > ClassPrepareEvent tests need to wait some time to allow handle all expected > events before terminate the debuggee. The logic was implemented by using > CountDownLatch. > > All tests are passed with "--test-repeat 20" Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: feedback - Changes: - all: https://git.openjdk.org/jdk/pull/18442/files - new: https://git.openjdk.org/jdk/pull/18442/files/cdcb4094..a30e3b70 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18442=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18442=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18442.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18442/head:pull/18442 PR: https://git.openjdk.org/jdk/pull/18442
RFR: JDK-8328303: 3 JDI tests timed out with UT enabled
The change fixes 3 nsk JDI tests. Root cause in all 3 tests is the same - the tests requests JDI event with SUSPEND_ALL policy, but event handler thread stops handle incoming event and this causes debuggee to hang (suspended by JDI event). All 3 tests are updated to exit event handler thread after getting VMDeathEvent or VMDisconnectEvent (and resume debuggee after any other events). ClassPrepareEvent tests need to wait some time to allow handle all expected events before terminate the debuggee. The logic was implemented by using CountDownLatch. All tests are passed with "--test-repeat 20" - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/18442/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18442=00 Issue: https://bugs.openjdk.org/browse/JDK-8328303 Stats: 93 lines in 3 files changed: 32 ins; 22 del; 39 mod Patch: https://git.openjdk.org/jdk/pull/18442.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18442/head:pull/18442 PR: https://git.openjdk.org/jdk/pull/18442
Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v7]
> RecordComponent class has _attributes_count field. > The only user of the field is JvmtiClassFileReconstituter. Incorrect value of > the field causes producing incorrect data for Record attribute. > Parsing Record attribute ClassFileParser skips unknown attributes and may > skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations. > Also annotations can be changed (added/removed) by class redefinition. > The fix removes attributes_count from RecordComponent; > JvmtiClassFileReconstituter calculates correct attributes_count generating > class bytes. > > Testing: > - tier1,tier2,hs-tier5-svc; > - redefineClasses/retransformClasses tests: >- test/jdk/java/lang/instrument >- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: added comment - Changes: - all: https://git.openjdk.org/jdk/pull/18161/files - new: https://git.openjdk.org/jdk/pull/18161/files/93256d90..84ad8e37 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18161=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18161=05-06 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18161.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18161/head:pull/18161 PR: https://git.openjdk.org/jdk/pull/18161
Re: RFR: 8328592: hprof tests fail with -XX:-CompactStrings [v2]
On Wed, 20 Mar 2024 17:19:34 GMT, Aleksey Shipilev wrote: >> See the bug for symptoms. The tests are failing because hprof test library >> is confused about non-compact strings. >> >> Additional testing: >> - [x] `serviceability/HeapDump lib-test:all` with `-XX:-CompactStrings` now >> pass >> - [x] `serviceability/HeapDump lib-test:all` with `-XX:+CompactStrings` >> still pass > > Aleksey Shipilev 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 three additional > commits since the last revision: > > - Handle platform endianness better > - Merge branch 'master' into JDK-8328592-hprof-compact-strings > - Fix Looks good. Please update copyright years before push - Marked as reviewed by amenkov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18394#pullrequestreview-1950543888
Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v5]
On Wed, 20 Mar 2024 04:40:52 GMT, Serguei Spitsyn wrote:
>> test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 107:
>>
>>> 105: newClassBytes = classBytes;
>>> 106: fInst.retransformClasses(targetClass);
>>> 107: assertTrue(targetClass.getName() + " was not seen by
>>> transform()", seenClassBytes != null);
>>
>> Nit: I guess, the `targetClassName` was intended to be used in place of
>> `targetClass.getName()`.
>
> The `retransform()` method returns an array of bytes. There is also a comment
> about it at the line 101. But the result has never been checked (please, see
> lines: 116, 123, 129, 135). This creates some confusion and questions why the
> return value is needed.
> The parameter `null` at lines 123, 129, 135 is also confusing and need some
> explanation.
> As I read the code, the method `retransform()` is saving the parameter
> `classBytes` in the `newClassBytes` field. The field `newClassBytes` value is
> then returned by the `transform()` method.
> But the `ClassFileTransformer.html#transform` states this:
> `Returns: a well-formed class file buffer (the result of the transform), or
> null if no transform is performed`.
>
> So that, when the `null` is returned by the `transform()` then it means there
> was no real transform.
> Could you comment on this? Do I miss anything here?
>
> Please, see:
> [ClassFileTransformer.html#transform](https://docs.oracle.com/en/java/javase/21/docs/api/java.instrument/java/lang/instrument/ClassFileTransformer.html#transform(java.lang.Module,java.lang.ClassLoader,java.lang.String,java.lang.Class,java.security.ProtectionDomain,byte%5B%5D))
>
> Also, the comment at line 122 is not needed at this form:
>
> // Ensure retransformation does not fail with ClassFormatError.
>
> Sorry, I was not clear when I've asked to add a comment at this point. I
> wanted a clarification about what does passing the `null` mean in terms of
> transformation. My understanding is that there is no real transformation when
> the `null` is returned but the `Instrumentation` mechanism is still working
> (for instance, the `JvmtiClassFileReconstituter` can be involved with an
> incorrect attributes counter) and can fail and we want to test/verify it.
> So, passing a Null-transformer is enough for our testing. Is it correct?
> Please, keep in mind, few people have some knowledge about the
> re-transformation details and would appreciate any hints here.
Instrumentation.retransformClasses logic can be described by the following
pseudo-code:
byte[] newClassBytes = JvmtiClassFileReconstituter.reconstitute(klass);
for (Transformer tr = firstTransformer(); tr != null; tr = tr->next()) {
byte[] transformerClassBytes = tr.transform(newClassBytes);
if (newClassBytes != null) {
newClassBytes = transformerClassBytes;
}
}
Then newClassBytes is parsed, verified and is used to redefine the class.
We need to verify that JvmtiClassFileReconstituter produces correct class
bytes, so test transformer can return null or passed classfileBuffer.
Actually the issue can be reproduced without ClassFileTransformer at all (and
this is mentioned in the CR) - in the case reconstituted class bytes is
considered as a result of the instrumentation.
I've updated the test, hope it's clearer now.
`targetClassName`/`seenClassBytes`/`newClassBytes` was used by Transformer, I
moved them to the class.
Please let me know if you think some additional comments would be useful.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1532968774
Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v6]
> RecordComponent class has _attributes_count field. > The only user of the field is JvmtiClassFileReconstituter. Incorrect value of > the field causes producing incorrect data for Record attribute. > Parsing Record attribute ClassFileParser skips unknown attributes and may > skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations. > Also annotations can be changed (added/removed) by class redefinition. > The fix removes attributes_count from RecordComponent; > JvmtiClassFileReconstituter calculates correct attributes_count generating > class bytes. > > Testing: > - tier1,tier2,hs-tier5-svc; > - redefineClasses/retransformClasses tests: >- test/jdk/java/lang/instrument >- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: updated the test - Changes: - all: https://git.openjdk.org/jdk/pull/18161/files - new: https://git.openjdk.org/jdk/pull/18161/files/9a53d4ca..93256d90 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18161=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18161=04-05 Stats: 29 lines in 1 file changed: 16 ins; 7 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18161.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18161/head:pull/18161 PR: https://git.openjdk.org/jdk/pull/18161
Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v5]
> RecordComponent class has _attributes_count field. > The only user of the field is JvmtiClassFileReconstituter. Incorrect value of > the field causes producing incorrect data for Record attribute. > Parsing Record attribute ClassFileParser skips unknown attributes and may > skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations. > Also annotations can be changed (added/removed) by class redefinition. > The fix removes attributes_count from RecordComponent; > JvmtiClassFileReconstituter calculates correct attributes_count generating > class bytes. > > Testing: > - tier1,tier2,hs-tier5-svc; > - redefineClasses/retransformClasses tests: >- test/jdk/java/lang/instrument >- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: Updated the fix and the test for multiple annotations - Changes: - all: https://git.openjdk.org/jdk/pull/18161/files - new: https://git.openjdk.org/jdk/pull/18161/files/671c71e1..9a53d4ca Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18161=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18161=03-04 Stats: 82 lines in 2 files changed: 58 ins; 11 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/18161.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18161/head:pull/18161 PR: https://git.openjdk.org/jdk/pull/18161
Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v3]
On Thu, 14 Mar 2024 02:12:29 GMT, Serguei Spitsyn wrote: > True. It should know how to put the `attribute_count` value into the class > file but it does not need to know how to calculate its value. What I do not > like in your model is that there is no one single place which knows how to > calculate this value and the existing and potential consumers should have > this knowledge. It **must** know how to calculate the value. Because this is number of `attribute_info_attributes` attributes that follow. And only `JvmtiClassFileReconstituter` knows how many attributes it's going to write. > The bug is that the `attributute_count` field value from the class file > stored in the `RecordComponent` does not match the calculated value because > the invisible attribute was not saved (was ignored). I'd consider this as design issue with the current implementation - `JvmtiClassFileReconstituter` gets the value from external source (`RecordComponent`) and uses it without validation. This approach is inconsistent with other `JvmtiClassFileReconstituter` code. For other similar cases it calculates record counts in place (from size of arrays, string length, from list enumerator, etc.). - PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1525542056
Integrated: JDK-8326898: NSK tests should listen on loopback addresses only
On Thu, 29 Feb 2024 01:50:02 GMT, Alex Menkov wrote: > Many NSK tests create socket channel for test/target interprocess > communication. > The change updates server side to listen only on loopback interface. > > Testing - all tests that use then functionality: > - test/hotspot/jtreg/vmTestbase/nsk/jdb, > test/hotspot/jtreg/vmTestbase/nsk/jdi, > test/hotspot/jtreg/vmTestbase/nsk/jdwp, > test/hotspot/jtreg/vmTestbase/nsk/aod, > test/hotspot/jtreg/vmTestbase/nsk/jvmti/AttachOnDemand This pull request has now been integrated. Changeset: 2482a505 Author:Alex Menkov URL: https://git.openjdk.org/jdk/commit/2482a505e5c898cc6365aa4fb8ca3e8b758b3a97 Stats: 22 lines in 6 files changed: 2 ins; 0 del; 20 mod 8326898: NSK tests should listen on loopback addresses only Reviewed-by: sspitsyn, cjplummer, dholmes, lmesnik - PR: https://git.openjdk.org/jdk/pull/18053
