Re: RFR: 8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING [v6]
On Thu, 8 Jun 2023 01:42:10 GMT, Serguei Spitsyn wrote: >> This is REDO the fix of >> [JDK-8307153](https://bugs.openjdk.org/browse/JDK-8307153). >> The last update of the fix in the review cycle was incorrect and incorrectly >> tested, so the issue has not been noticed. It is why the fix was backed out. >> The issue is that the SUSPEND bit was missed in the JVMTI thread state of >> platform/carrier threads carrying virtual threads >> (see`JvmtiEnvBase::get_thread_state` function). >> >> The first push/patch is the original fix of JDK-8307153. >> The fix of the SUSPEND bit issue will be in the incremental update. >> It is to simplify the review. >> >> Testing: >> - TBD: mach5 tiers 1-5 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: corrected the function get_thread_state for safety Chris and Alex, thank you for review! - PR Comment: https://git.openjdk.org/jdk/pull/14366#issuecomment-1584039912
Integrated: 8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING
On Wed, 7 Jun 2023 18:42:34 GMT, Serguei Spitsyn wrote: > This is REDO the fix of > [JDK-8307153](https://bugs.openjdk.org/browse/JDK-8307153). > The last update of the fix in the review cycle was incorrect and incorrectly > tested, so the issue has not been noticed. It is why the fix was backed out. > The issue is that the SUSPEND bit was missed in the JVMTI thread state of > platform/carrier threads carrying virtual threads > (see`JvmtiEnvBase::get_thread_state` function). > > The first push/patch is the original fix of JDK-8307153. > The fix of the SUSPEND bit issue will be in the incremental update. > It is to simplify the review. > > Testing: > - TBD: mach5 tiers 1-5 This pull request has now been integrated. Changeset: f91e9ba7 Author:Serguei Spitsyn URL: https://git.openjdk.org/jdk/commit/f91e9ba757f04983655c23542e06973805465249 Stats: 96 lines in 4 files changed: 76 ins; 0 del; 20 mod 8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING Reviewed-by: cjplummer, amenkov - PR: https://git.openjdk.org/jdk/pull/14366
Re: RFR: 8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING [v4]
On Thu, 8 Jun 2023 06:25:20 GMT, Alan Bateman wrote: >> It was decided with Alan that it is okay to be in a waiting state. The >> `JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER` state requires a monitor to be >> blocked on, so it can be confusing. Alan's comment in the original PR >> [https://github.com/openjdk/jdk/pull/14298](https://github.com/openjdk/jdk/pull/14298) >> was: >>> if the jt is carrying thread_oop and it's okay for the JVMTI state to >>> reported as WAITING when waiting for something other than Object.wait. > > The mental model is that the carrier is blocked so this is what an observer > using the APIs should see. My recollection is that JVMTI_THREAD_STATE_WAITING > was okay because there is a wriggle room in the JVM TI spec, it only uses > Object.wait as an example. There may be a few rough edges to smooth down in > this area. It's okay to take time with this PR and expand the tests to cover > more cases and get more confident that there aren't more issues. We agreed with Alex to file a test RFE to improve test coverage in this area. - PR Review Comment: https://git.openjdk.org/jdk/pull/14366#discussion_r1223883294
Re: RFR: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
On Thu, 8 Jun 2023 17:14:39 GMT, Yudi Zheng wrote:
> HeapMonitor checks if System.getProperty("jvmci.Compiler") is graal and will
> not enforce checking line number derived from uncommon trap debug info.
> However, Graal does not set this property explicitly.
Looks good.
Is JVMCI used by the Graal compiler only?
Thanks,
Serguei
-
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14381#pullrequestreview-1471250105
Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v8]
On Thu, 8 Jun 2023 19:05:54 GMT, Doug Simon wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> update copyright comments > > src/hotspot/share/runtime/sharedRuntime.cpp line 641: > >> 639: JRT_ENTRY(void, SharedRuntime::notify_jvmti_vthread_start(oopDesc* vt, >> jboolean hide, JavaThread* current)) >> 640: assert(hide == JNI_FALSE, "must be VTMS transition finish"); >> 641: jobject vthread = JNIHandles::make_local(const_cast(vt)); > > Since the current thread is in the `current` arg, it could be used here when > creating the local handle. That's right. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/13484#discussion_r1223851214
RFR: 8309673: Refactor ref_at methods in Serviceability ConstantPool
The accessor methods in constantpool.cpp were previously cleaned up to allow for different types of indices to be used, distinguishing them by the bytecode. This patch adds the same changes to the hotspot serviceability code. Verified with tier 1-5 tests. - Commit messages: - 8309673: Refactor ref_at methods in Serviceability ConstantPool Changes: https://git.openjdk.org/jdk/pull/14385/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14385&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309673 Stats: 95 lines in 4 files changed: 32 ins; 25 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/14385.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14385/head:pull/14385 PR: https://git.openjdk.org/jdk/pull/14385
Re: RFR: 8232839: JDI AfterThreadDeathTest.java failed due to "FAILED: Did not get expected IllegalThreadStateException on a StepRequest.enable()"
On Wed, 7 Jun 2023 23:17:18 GMT, Chris Plummer wrote:
> The test waits for a ThreadDeathEvent for "main". Once that arrives, it then
> waits for the next ThreadStartEvent (for any thread). Once it arrives, the
> test tries to create and enable a StepRequest on the "main" thread. Since
> "main" is supposedly dead, the expectation is an IllegalThreadStateException.
> However, it turns out that sometimes the enabling can in fact succeed.
>
> Just because a ThreadDeathEvent has been received for a thread does not mean
> you can no longer do things with the thread like suspend it or enable a
> StepEvent. There is a short delay in the debug agent after sending the
> ThreadDeathEvent before it stops tracking the thread. The thread can still be
> acted upon until then. The JDWP and JDI specs seem to support doing this:
>
>> Notification of a completed thread in the target VM. The notification is
>> generated by the dying thread before it terminates. Because of this timing,
>> it is possible for {@link VirtualMachine#allThreads} to return this thread
>> after this event is received.
>>
>> Note that this event gives no information about the lifetime of the thread
>> object. It may or may not be collected soon depending on what references
>> exist in the target VM.
>
> What this means is that when the test receives some arbitrary
> ThreadStartEvent after the "main" ThreadDeathEvent has been received, the
> test may in fact still be able to enable a StepRequest on the "main" thread,
> causing the test to fail. What seems to trigger the failure is receiving an
> unexpected spurious ThreadStartEvent such as from the Common-Clean thread or
> a carrier thread, although even then it only fails some of the time. In fact
> if I modify the test to enable the StepRequest when it receives the
> ThreadDeathEvent for "main", it still almost always passes, but will fail
> more frequently than it normally does.
>
> It seems if the test always waits for the ThreadStartEvent for
> "DestroyJavaVM", then the "main" thread is truly gone by then and the test
> always passes, so this is how I've chosen to fix the issue.
>
> Tested with tier1, tier2 svc tests, and tier5 svc tests.
I think it makes sense!
-
Marked as reviewed by kevinw (Committer).
PR Review: https://git.openjdk.org/jdk/pull/14372#pullrequestreview-1470630717
Re: RFR: 8309271: A way to align already compiled methods with compiler directives
On Wed, 24 May 2023 00:38:27 GMT, Dmitry Chuyko wrote: > Compiler Control (https://openjdk.org/jeps/165) provides method-context > dependent control of the JVM compilers (C1 and C2). The active directive > stack is built from the directive files passed with the > `-XX:CompilerDirectivesFile` diagnostic command-line option and the > Compiler.add_directives diagnostic command. It is also possible to clear all > directives or remove the top from the stack. > > A matching directive will be applied at method compilation time when such > compilation is started. If directives are added or changed, but compilation > does not start, then the state of compiled methods doesn't correspond to the > rules. This is not an error, and it happens in long running applications when > directives are added or removed after compilation of methods that could be > matched. For example, the user decides that C2 compilation needs to be > disabled for some method due to a compiler bug, issues such a directive but > this does not affect the application behavior. In such case, the target > application needs to be restarted, and such an operation can have high costs > and risks. Another goal is testing/debugging compilers. > > It would be convenient to optionally reconcile at least existing matching > nmethods to the current stack of compiler directives. Methods in general are > often inlined, and this information is hard to track down. > > Natural way to eliminate the discrepancy between the result of compilation > and the broken rule is to discard the compilation result, i.e. > deoptimization. Obviously there is a performance penalty, so it should be > applied with care. Hot code will most likely be recompiled soon, as nothing > happens to its hotness. > > A new flag '`-d`' has beed introduced for some directives related to compile > commands: `Compiler.add_directives`, `Compiler.remove_directives`, > `Compiler.clear_directives`. The default behavior has not changed (no flag). > If the new flag is present, the command scans already compiled methods and > marks for deoptimization those methods that have any active non-default > matching compiler directives. There is currently no distinction which > directives are found. In particular, this means that if there are rules for > inlining into some method, it will be deoptimized. On the other hand, if > there are rules for a method and it was inlined, top-level methods won't be > deoptimized, but this can be achieved by having rules for them. > > In addition, a new diagnistic command `Compiler.replace_directives`, has been > added for convenience. It's like a combinatio... "refresh" (-r) would be better than "deoptimize" (-d). The latter implies a specific implementation, the former is generic. If the method is to be recompiled, perhaps rather than deopt and wait, add it to the compile queue immediately and deopt the old version when the new compilation is complete, similar to what happens when the c1 version of the method is replaced by the c2 version. - PR Comment: https://git.openjdk.org/jdk/pull/14111#issuecomment-1583199824
Re: RFR: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
On Thu, 8 Jun 2023 18:29:50 GMT, Yudi Zheng wrote:
>> test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor.java
>> line 257:
>>
>>> 255:
>>> 256: checkLines = !(enableJVMCI.getValue().equals("true")
>>> 257: && useJVMCICompiler.getValue().equals("true"));
>>
>> Is it possible to use `jdk.test.whitebox.code.Compiler.isGraalEnabled()`
>> here instead?
>
> To use whitebox I will have to update the config of every test here, which I
> think is too verbose:
>
> diff --git
> a/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
>
> b/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
> index e08454a4857..f086f744965 100644
> ---
> a/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
> +++
> b/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
> @@ -27,11 +27,15 @@ package MyPackage;
> /**
> * @test
> * @summary Verifies the JVMTI Heap Monitor sampling interval average.
> - * @build Frame HeapMonitor
> + * @library /test/lib
> + * @build Frame HeapMonitor jdk.test.whitebox.WhiteBox
> * @compile HeapMonitorStatIntervalTest.java
> * @requires vm.jvmti
> * @requires vm.compMode != "Xcomp"
> - * @run main/othervm/native -agentlib:HeapMonitorTest
> MyPackage.HeapMonitorStatIntervalTest
> + * @run driver jdk.test.lib.helpers.ClassFileInstaller
> jdk.test.whitebox.WhiteBox
> + * @run main/othervm/native -agentlib:HeapMonitorTest -Xbootclasspath/a:.
> + * -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
> + * MyPackage.HeapMonitorStatIntervalTest
> */
Ok.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14381#discussion_r1223447124
Re: RFR: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
On Thu, 8 Jun 2023 17:14:39 GMT, Yudi Zheng wrote:
> HeapMonitor checks if System.getProperty("jvmci.Compiler") is graal and will
> not enforce checking line number derived from uncommon trap debug info.
> However, Graal does not set this property explicitly.
Marked as reviewed by dnsimon (Committer).
-
PR Review: https://git.openjdk.org/jdk/pull/14381#pullrequestreview-1470601144
Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v8]
On Tue, 2 May 2023 02:01:44 GMT, Serguei Spitsyn wrote: >> This refactoring to separate ThreadStart/ThreadEnd events posting code in >> the JVMTI VTMS transitions is needed for future work on JVMTI scalability >> and performance improvements. It is to easier put this code on slow path. >> >> Testing: mach5 tiers 1-6 were successful. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > update copyright comments src/hotspot/share/runtime/sharedRuntime.cpp line 641: > 639: JRT_ENTRY(void, SharedRuntime::notify_jvmti_vthread_start(oopDesc* vt, > jboolean hide, JavaThread* current)) > 640: assert(hide == JNI_FALSE, "must be VTMS transition finish"); > 641: jobject vthread = JNIHandles::make_local(const_cast(vt)); Since the current thread is in the `current` arg, it could be used here when creating the local handle. - PR Review Comment: https://git.openjdk.org/jdk/pull/13484#discussion_r1223444559
Re: RFR: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
On Thu, 8 Jun 2023 17:19:46 GMT, Doug Simon wrote:
>> HeapMonitor checks if System.getProperty("jvmci.Compiler") is graal and will
>> not enforce checking line number derived from uncommon trap debug info.
>> However, Graal does not set this property explicitly.
>
> test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor.java
> line 257:
>
>> 255:
>> 256: checkLines = !(enableJVMCI.getValue().equals("true")
>> 257: && useJVMCICompiler.getValue().equals("true"));
>
> Is it possible to use `jdk.test.whitebox.code.Compiler.isGraalEnabled()` here
> instead?
To use whitebox I will have to update the config of every test here, which I
think is too verbose:
diff --git
a/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
b/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
index e08454a4857..f086f744965 100644
---
a/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
+++
b/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
@@ -27,11 +27,15 @@ package MyPackage;
/**
* @test
* @summary Verifies the JVMTI Heap Monitor sampling interval average.
- * @build Frame HeapMonitor
+ * @library /test/lib
+ * @build Frame HeapMonitor jdk.test.whitebox.WhiteBox
* @compile HeapMonitorStatIntervalTest.java
* @requires vm.jvmti
* @requires vm.compMode != "Xcomp"
- * @run main/othervm/native -agentlib:HeapMonitorTest
MyPackage.HeapMonitorStatIntervalTest
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller
jdk.test.whitebox.WhiteBox
+ * @run main/othervm/native -agentlib:HeapMonitorTest -Xbootclasspath/a:.
+ * -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
+ * MyPackage.HeapMonitorStatIntervalTest
*/
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14381#discussion_r122345
Re: RFR: 8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING [v6]
On Thu, 8 Jun 2023 01:42:10 GMT, Serguei Spitsyn wrote: >> This is REDO the fix of >> [JDK-8307153](https://bugs.openjdk.org/browse/JDK-8307153). >> The last update of the fix in the review cycle was incorrect and incorrectly >> tested, so the issue has not been noticed. It is why the fix was backed out. >> The issue is that the SUSPEND bit was missed in the JVMTI thread state of >> platform/carrier threads carrying virtual threads >> (see`JvmtiEnvBase::get_thread_state` function). >> >> The first push/patch is the original fix of JDK-8307153. >> The fix of the SUSPEND bit issue will be in the incremental update. >> It is to simplify the review. >> >> Testing: >> - TBD: mach5 tiers 1-5 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: corrected the function get_thread_state for safety Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14366#pullrequestreview-1470533353
Re: RFR: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
On Thu, 8 Jun 2023 17:14:39 GMT, Yudi Zheng wrote:
> HeapMonitor checks if System.getProperty("jvmci.Compiler") is graal and will
> not enforce checking line number derived from uncommon trap debug info.
> However, Graal does not set this property explicitly.
test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor.java
line 257:
> 255:
> 256: checkLines = !(enableJVMCI.getValue().equals("true")
> 257: && useJVMCICompiler.getValue().equals("true"));
Is it possible to use `jdk.test.whitebox.code.Compiler.isGraalEnabled()` here
instead?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14381#discussion_r1223342262
RFR: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
HeapMonitor checks if System.getProperty("jvmci.Compiler") is graal and will
not enforce checking line number derived from uncommon trap debug info.
However, Graal does not set this property explicitly.
-
Commit messages:
- Avoid using jvmci.Compiler property to determine if Graal is enabled.
Changes: https://git.openjdk.org/jdk/pull/14381/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14381&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8309671
Stats: 3 lines in 2 files changed: 0 ins; 2 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/14381.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14381/head:pull/14381
PR: https://git.openjdk.org/jdk/pull/14381
Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental) [v17]
On Wed, 7 Jun 2023 21:03:47 GMT, Kelvin Nilsen wrote: > We would like to thank everyone who has taken time to review and provide > feedback on our pull request. Given the risks identified during the review > process and the lack of time available to perform the thorough review that > such a large contribution of code requires, we have decided to close this PR > at the current time. We will seek to target JDK 22. Thank you for this. It's the right decision. In hindsight, there never was a highly-likely prospect of getting such a substantial and interwoven patch successfully reviewed in such a short time, even with the most skilful and experienced team. - PR Comment: https://git.openjdk.org/jdk/pull/14185#issuecomment-1582124942
Re: RFR: 8305341: Alignment should be enforced by alignas instead of compiler specific attributes [v4]
On Wed, 12 Apr 2023 07:12:10 GMT, Julian Waters wrote: >> C11 has been stable for a long time on all platforms, so native code can use >> the standard alignas operator for alignment requirements > > Julian Waters has updated the pull request incrementally with four additional > commits since the last revision: > > - Restore visCPP > - Restore gcc attribute > - Revert gcc > - Revert Anyone? - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1582065715
