Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v5]

[Serguei Spitsyn]

On Wed, 20 Mar 2024 22:24:37 GMT, Alex Menkov  wrote:

>> 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.

Thank you for the update, it is a nice change.
I'd also suggest to add a comment before line 115, something like this:

  // Below the null is passed as the classBytes argument which means there 
won't be any transformation.
  // However, it is enough for testing purposes as the 
JvmtiClassFileReconstituter still involved
  // into preparation of the initial classfile bytecodes.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1533251745

Re: RFR: 8328592: hprof tests fail with -XX:-CompactStrings [v2]

[Paul Hohensee]

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

Marked as reviewed by phh (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18394#pullrequestreview-1950715132

Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v5]

[Leonid Mesnik]

On Thu, 21 Mar 2024 01:09:52 GMT, Serguei Spitsyn  wrote:

>> The `JvmtiHandshake` and `JvmtiUnitedHandshakeClosure` classes were 
>> introduced in the JDK 22 to unify/simplify the JVM TI functions supporting 
>> implementation of the virtual threads. This enhancement is to refactor JVM 
>> TI functions `GetOwnedMonitorInfo` and `GetOwnedMonitorStackDepthInfo` on 
>> the base of `JvmtiHandshake and `JvmtiUnitedHandshakeClosure` classes.
>> 
>> Testing:
>>  - Ran mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review: replace redundand check with an assert

Marked as reviewed by lmesnik (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18332#pullrequestreview-1950561720

Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v4]

[Leonid Mesnik]

On Thu, 21 Mar 2024 01:01:22 GMT, Serguei Spitsyn  wrote:

>> src/hotspot/share/prims/jvmtiEnv.cpp line 1366:
>> 
>>> 1364:   }
>>> 1365: 
>>> 1366:   if (java_thread != nullptr) {
>> 
>> I am not sure why this check is needed at this level. It is duplication.The 
>> GetOwnedMonitorInfoClosure::do_vthread checks if thread is null (no-op for 
>> unmounted threads). 
>> Currently, the unmounted threads can't own monitor. But if it's changed we 
>> need to update this check as well as the implementation of handshakes.
>
> This is needed to avoid using the `EscapeBarrier` for unmounted virtual 
> threads.
> But you are right there is a duplication here - thanks.
> The following check can be removed:
> 
> +GetOwnedMonitorInfoClosure::do_vthread(Handle target_h) {
> +  if (_target_jt == nullptr) {
> +_result = JVMTI_ERROR_NONE;
> +return;
> +  }
> 
> I've replaced it with an assert:
> 
>   assert(_target_jt != nullptr, "sanity check");

Thanks, that should work!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18332#discussion_r1533118435

Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v5]

[Serguei Spitsyn]

> The `JvmtiHandshake` and `JvmtiUnitedHandshakeClosure` classes were 
> introduced in the JDK 22 to unify/simplify the JVM TI functions supporting 
> implementation of the virtual threads. This enhancement is to refactor JVM TI 
> functions `GetOwnedMonitorInfo` and `GetOwnedMonitorStackDepthInfo` on the 
> base of `JvmtiHandshake and `JvmtiUnitedHandshakeClosure` classes.
> 
> Testing:
>  - Ran mach5 tiers 1-6

Serguei Spitsyn has updated the pull request incrementally with one additional 
commit since the last revision:

  review: replace redundand check with an assert

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18332/files
  - new: https://git.openjdk.org/jdk/pull/18332/files/b7380b19..429e696a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18332=04
 - incr: https://webrevs.openjdk.org/?repo=jdk=18332=03-04

  Stats: 4 lines in 1 file changed: 0 ins; 3 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/18332.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18332/head:pull/18332

PR: https://git.openjdk.org/jdk/pull/18332

Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v4]

[Serguei Spitsyn]

On Wed, 20 Mar 2024 22:41:05 GMT, Leonid Mesnik  wrote:

>> Serguei Spitsyn has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   review: work around problem for vthreads in 
>> EscapeBarrier::deoptimize_objects
>
> src/hotspot/share/prims/jvmtiEnv.cpp line 1366:
> 
>> 1364:   }
>> 1365: 
>> 1366:   if (java_thread != nullptr) {
> 
> I am not sure why this check is needed at this level. It is duplication.The 
> GetOwnedMonitorInfoClosure::do_vthread checks if thread is null (no-op for 
> unmounted threads). 
> Currently, the unmounted threads can't own monitor. But if it's changed we 
> need to update this check as well as the implementation of handshakes.

This is needed to avoid using the `EscapeBarrier` for unmounted virtual threads.
But you are right there is a duplication here - thanks.
The following check can be removed:

+GetOwnedMonitorInfoClosure::do_vthread(Handle target_h) {
+  if (_target_jt == nullptr) {
+_result = JVMTI_ERROR_NONE;
+return;
+  }

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18332#discussion_r1533109911

Re: RFR: 8328592: hprof tests fail with -XX:-CompactStrings [v2]

[Alex Menkov]

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: 8327505: Test com/sun/jmx/remote/NotificationMarshalVersions/TestSerializationMismatch.java fails

[Leonid Mesnik]

On Tue, 19 Mar 2024 16:23:27 GMT, Kevin Walls  wrote:

> Client.java has a fixed 30-second timeout on the CountDownLatch to wait for 
> 10 notifications.
> 
> If it fails, you can't tell if CountDownLatch.await threw, or returned false 
> and the app threw InterruptedException, due to the way Client.java handles 
> these.
> 
> Seems most likely the 30 second wait expired, as we are dealing with -Xcomp 
> failures in a debug build.  Passing runs can take a few seconds, but can be 
> 25 seconds.
> 
> Increasing the timeout and tidying up the handling so we can see the specific 
> reason in future.

Marked as reviewed by lmesnik (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18381#pullrequestreview-1950389559

Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v4]

[Leonid Mesnik]

On Wed, 20 Mar 2024 22:03:30 GMT, Serguei Spitsyn  wrote:

>> The `JvmtiHandshake` and `JvmtiUnitedHandshakeClosure` classes were 
>> introduced in the JDK 22 to unify/simplify the JVM TI functions supporting 
>> implementation of the virtual threads. This enhancement is to refactor JVM 
>> TI functions `GetOwnedMonitorInfo` and `GetOwnedMonitorStackDepthInfo` on 
>> the base of `JvmtiHandshake and `JvmtiUnitedHandshakeClosure` classes.
>> 
>> Testing:
>>  - Ran mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review: work around problem for vthreads in 
> EscapeBarrier::deoptimize_objects

src/hotspot/share/prims/jvmtiEnv.cpp line 1366:

> 1364:   }
> 1365: 
> 1366:   if (java_thread != nullptr) {

I am not sure why this check is needed at this level. It is duplication.The 
GetOwnedMonitorInfoClosure::do_vthread checks if thread is null (no-op for 
unmounted threads). 
Currently, the unmounted threads can't own monitor. But if it's changed we need 
to update this check as well as the implementation of handshakes.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18332#discussion_r1532988358

Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v5]

[Alex Menkov]

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]

[Alex Menkov]

> 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: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v4]

[Serguei Spitsyn]

> The `JvmtiHandshake` and `JvmtiUnitedHandshakeClosure` classes were 
> introduced in the JDK 22 to unify/simplify the JVM TI functions supporting 
> implementation of the virtual threads. This enhancement is to refactor JVM TI 
> functions `GetOwnedMonitorInfo` and `GetOwnedMonitorStackDepthInfo` on the 
> base of `JvmtiHandshake and `JvmtiUnitedHandshakeClosure` classes.
> 
> Testing:
>  - Ran mach5 tiers 1-6

Serguei Spitsyn has updated the pull request incrementally with one additional 
commit since the last revision:

  review: work around problem for vthreads in EscapeBarrier::deoptimize_objects

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18332/files
  - new: https://git.openjdk.org/jdk/pull/18332/files/ebbf0a0f..b7380b19

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18332=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=18332=02-03

  Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/18332.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18332/head:pull/18332

PR: https://git.openjdk.org/jdk/pull/18332

Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v2]

[Serguei Spitsyn]

On Wed, 20 Mar 2024 15:59:34 GMT, Patricio Chilano Mateo 
 wrote:

>> Good suggestion, thanks!
>> Would the following fix work ? :
>> 
>> 
>> git diff
>> diff --git a/src/hotspot/share/runtime/escapeBarrier.cpp 
>> b/src/hotspot/share/runtime/escapeBarrier.cpp
>> index bc01d900285..1b6d57644dc 100644
>> --- a/src/hotspot/share/runtime/escapeBarrier.cpp
>> +++ b/src/hotspot/share/runtime/escapeBarrier.cpp
>> @@ -75,7 +75,7 @@ bool EscapeBarrier::deoptimize_objects(int d1, int d2) {
>>  // These frames are about to be removed. We must not interfere with 
>> that and signal failure.
>>  return false;
>>}
>> -  if (deoptee_thread()->has_last_Java_frame()) {
>> +  if (deoptee_thread()->has_last_Java_frame() && 
>> deoptee_thread()->last_continuation() == nullptr) {
>>  assert(calling_thread() == Thread::current(), "should be");
>>  KeepStackGCProcessedMark ksgcpm(deoptee_thread());
>>  ResourceMark rm(calling_thread());
>> 
>> 
>> BTW, it seems the `PopFrame` and the `ForceEarlyReturn` are broken 
>> the same way and, I hope, the fix above should solve the issue.
>
> Fix looks good.

Thanks!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18332#discussion_r1532932393

Integrated: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs

[Weijun Wang]

On Wed, 17 Jan 2024 23:41:53 GMT, Weijun Wang  wrote:

> This code change adds an alternative implementation of user-based 
> authorization `Subject` APIs that doesn't depend on Security Manager APIs. 
> Depending on if the Security Manager is allowed, the methods store the 
> current subject differently. See the spec change in the `Subject.java` file 
> for details. When the Security Manager APIs are finally removed in a future 
> release, this new implementation will be only implementation for these 
> methods.
> 
> One major change in the new implementation is that `Subject.getSubject` 
> always throws an `UnsupportedOperationException` since it has an 
> `AccessControlContext` argument but the current subject is no longer 
> associated with an `AccessControlContext` object.
> 
> Now it's the time to migrate from the `getSubject` and `doAs` methods to 
> `current` and `callAs`. If the user application is simply calling 
> `getSubject(AccessController.getContext())`, then switching to `current()` 
> would work. If the `AccessControlContext` argument is retrieved from an 
> earlier `getContext()` call and the associated subject might be different 
> from that of the current `AccessControlContext`, then instead of storing the 
> previous `AccessControlContext` object and passing it into `getSubject` to 
> get the "previous" subject, the application should store the `current()` 
> return value directly.

This pull request has now been integrated.

Changeset: d32746ef
Author:Weijun Wang 
URL:   
https://git.openjdk.org/jdk/commit/d32746ef4a0ce6fec558274244321991be141698
Stats: 622 lines in 17 files changed: 477 ins; 27 del; 118 mod

8296244: Alternate implementation of user-based authorization Subject APIs that 
doesn’t depend on Security Manager APIs

Reviewed-by: mullan

-

PR: https://git.openjdk.org/jdk/pull/17472

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v8]

[Sean Mullan]

On Wed, 20 Mar 2024 14:45:50 GMT, Weijun Wang  wrote:

>> This code change adds an alternative implementation of user-based 
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs. 
>> Depending on if the Security Manager is allowed, the methods store the 
>> current subject differently. See the spec change in the `Subject.java` file 
>> for details. When the Security Manager APIs are finally removed in a future 
>> release, this new implementation will be only implementation for these 
>> methods.
>> 
>> One major change in the new implementation is that `Subject.getSubject` 
>> always throws an `UnsupportedOperationException` since it has an 
>> `AccessControlContext` argument but the current subject is no longer 
>> associated with an `AccessControlContext` object.
>> 
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to 
>> `current` and `callAs`. If the user application is simply calling 
>> `getSubject(AccessController.getContext())`, then switching to `current()` 
>> would work. If the `AccessControlContext` argument is retrieved from an 
>> earlier `getContext()` call and the associated subject might be different 
>> from that of the current `AccessControlContext`, then instead of storing the 
>> previous `AccessControlContext` object and passing it into `getSubject` to 
>> get the "previous" subject, the application should store the `current()` 
>> return value directly.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   more allow and years

Marked as reviewed by mullan (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/17472#pullrequestreview-1950109209

Re: RFR: 8328592: hprof tests fail with -XX:-CompactStrings [v2]

[Aleksey Shipilev]

On Wed, 20 Mar 2024 14:46:09 GMT, Paul Hohensee  wrote:

> Are we guaranteed that non-compact string char component bytes are stored in 
> little-endian order?

No, I don't think we are guaranteed any particular endianness, argh. The new 
patch follows what `StringUTF16` does: 
https://github.com/openjdk/jdk/blob/4e83f4cfc779e39cca0070b5729a508aeaa74654/src/java.base/share/classes/java/lang/StringUTF16.java#L1663-L1673

-

PR Comment: https://git.openjdk.org/jdk/pull/18394#issuecomment-2010124301

Re: RFR: 8328592: hprof tests fail with -XX:-CompactStrings [v2]

[Aleksey Shipilev]

> 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

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18394/files
  - new: https://git.openjdk.org/jdk/pull/18394/files/5ba1f4b8..f783418d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18394=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=18394=00-01

  Stats: 292803 lines in 402 files changed: 5222 ins; 5761 del; 281820 mod
  Patch: https://git.openjdk.org/jdk/pull/18394.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18394/head:pull/18394

PR: https://git.openjdk.org/jdk/pull/18394

Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v2]

[Patricio Chilano Mateo]

On Wed, 20 Mar 2024 08:12:06 GMT, Serguei Spitsyn  wrote:

>> src/hotspot/share/prims/jvmtiEnv.cpp line 1368:
>> 
>>> 1366:   if (java_thread != nullptr) {
>>> 1367: Handle thread_handle(calling_thread, thread_oop);
>>> 1368: EscapeBarrier eb(true, calling_thread, java_thread);
>> 
>> I see that now we will execute the EscapeBarrier code for the vthread case 
>> too. We actually had to disable that for virtual threads since it doesn't 
>> work (8285739 and 8305625). But we only addressed the case when targeting 
>> all threads and not the single thread one. We would need to add the same 
>> check in EscapeBarrier::deoptimize_objects(int d1, int d2) to skip a thread 
>> with mounted continuation.
>
> Good suggestion, thanks!
> Would the following fix work ? :
> 
> 
> git diff
> diff --git a/src/hotspot/share/runtime/escapeBarrier.cpp 
> b/src/hotspot/share/runtime/escapeBarrier.cpp
> index bc01d900285..1b6d57644dc 100644
> --- a/src/hotspot/share/runtime/escapeBarrier.cpp
> +++ b/src/hotspot/share/runtime/escapeBarrier.cpp
> @@ -75,7 +75,7 @@ bool EscapeBarrier::deoptimize_objects(int d1, int d2) {
>  // These frames are about to be removed. We must not interfere with that 
> and signal failure.
>  return false;
>}
> -  if (deoptee_thread()->has_last_Java_frame()) {
> +  if (deoptee_thread()->has_last_Java_frame() && 
> deoptee_thread()->last_continuation() == nullptr) {
>  assert(calling_thread() == Thread::current(), "should be");
>  KeepStackGCProcessedMark ksgcpm(deoptee_thread());
>  ResourceMark rm(calling_thread());
> 
> 
> BTW, it seems the `PopFrame` and the `ForceEarlyReturn` are broken 
> the same way and, I hope, the fix above should solve the issue.

Fix looks good.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18332#discussion_r1532363608

Re: RFR: 8328592: hprof tests fail with -XX:-CompactStrings

[Leonid Mesnik]

On Wed, 20 Mar 2024 09:53:36 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

Marked as reviewed by lmesnik (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18394#pullrequestreview-1949282694

RE: RFD: Remove Hotspot-internal MBeans from sun.management

[Kevin Walls]

Hi,

8328618: HotspotInternalMBean should be removed
https://bugs.openjdk.org/browse/JDK-8328618

I looked a bit further and logged this, with notes on what the HotSpotXX MBeans 
actually do.  It is almost all about providing a Java MBean interface to perf 
counters. HotspotThread additionally provides thread count and thread CPU usage 
by “internal” threads, although the definition of that is maybe unclear.

Thanks
Kevin


From: Kevin Walls
Sent: 20 March 2024 10:53
To: Mandy Chung ; Eirik Bjørsnøs ; 
serviceability-dev@openjdk.org
Cc: Volker Simonis ; Alan Bateman 

Subject: RE: RFD: Remove Hotspot-internal MBeans from sun.management


Yes, I looked into it a little since you raised the previous issue, that looks 
like the right direction.

HotspotInternalMBean should be removed (an undocumented, unsupported and 
experimental interface)

HotspotInternalMBean creates: sun.management….
HotspotClassLoading
HotspotMemory
HotspotRuntime
HotspotThread
HotspotCompilation

These have a peculiar way of accessing that we do not document as far as I can 
see.

It would be good to look at whether anything in there is not adequately 
available by other MBeans, or jstat, or any other mechanism.

Thanks
Kevin


From: Mandy Chung mailto:mandy.ch...@oracle.com>>
Sent: 19 March 2024 16:48
To: Eirik Bjørsnøs mailto:eir...@gmail.com>>; 
serviceability-dev@openjdk.org
Cc: Volker Simonis mailto:volker.simo...@gmail.com>>; 
Kevin Walls mailto:kevin.wa...@oracle.com>>; Alan 
Bateman mailto:alan.bate...@oracle.com>>
Subject: Re: RFD: Remove Hotspot-internal MBeans from sun.management


Would a PR to remove these APIs be welcome?

Good with me.

Mandy
On 3/19/24 9:41 AM, Eirik Bjørsnøs wrote:

Hi,

Last September, Volker shared the observation that we have Hotspot-internal 
MBeans in sun.management which are strongly encapsulated and not used 
internally by OpenJDK besides their unit tests:

https://www.mail-archive.com/core-libs-dev@openjdk.org/msg19878.html

A summary of the email thread:

Mandy pointed out:

We added these HotSpot internal MBeans in JDK 5 to expose the internal metrics. 
 Most of these internal metrics are exposed via jstat tool too.   We didn't 
receive much feedback regarding these HotSpot internal MBeans.Removing them 
is fine and good cleanup effort.

Alan made a similar point:

It's left over from experiments on exposing some internal metrics, I think 
during JDK 5. It's code that should probably have been removed a long time ago.

Kirk P raised a concern:

It would be a shame to lose these metrics because many of them have been very
useful over time and some would be even more useful with some modifications.

To which Mandy responded:

What we're referring to is to remove sun.management.Hotspot*, the internal 
MBeans which are never exposed and registered in the platform MBeanServer.   
The internal metrics in HotSpot VM should be retained as they are exposed 
through other ways like jstat, GC logs, etc.

The email thread seems to have ended here without further action taken.

My interpretation of the above is that we have a consensus that these 
Hotspot-internal MBeans can be removed. Since I was not part of the initial 
discussion and some time has passed, I'd like some confirmation that my 
interpretation is correct.

Would a PR to remove these APIs be welcome?

(This would remove HotspotInternalMBean, HotspotMemoryMBean, 
HotspotRuntimeMBean, HotspotThreadMBean, with associated implementation, 
factory methods, tests and probably also some native code in libmanagement. 
Details can be discussed in a PR)

Cheers,
Eirik.

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v8]

[Weijun Wang]

On Wed, 20 Mar 2024 14:45:50 GMT, Weijun Wang  wrote:

>> This code change adds an alternative implementation of user-based 
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs. 
>> Depending on if the Security Manager is allowed, the methods store the 
>> current subject differently. See the spec change in the `Subject.java` file 
>> for details. When the Security Manager APIs are finally removed in a future 
>> release, this new implementation will be only implementation for these 
>> methods.
>> 
>> One major change in the new implementation is that `Subject.getSubject` 
>> always throws an `UnsupportedOperationException` since it has an 
>> `AccessControlContext` argument but the current subject is no longer 
>> associated with an `AccessControlContext` object.
>> 
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to 
>> `current` and `callAs`. If the user application is simply calling 
>> `getSubject(AccessController.getContext())`, then switching to `current()` 
>> would work. If the `AccessControlContext` argument is retrieved from an 
>> earlier `getContext()` call and the associated subject might be different 
>> from that of the current `AccessControlContext`, then instead of storing the 
>> previous `AccessControlContext` object and passing it into `getSubject` to 
>> get the "previous" subject, the application should store the `current()` 
>> return value directly.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   more allow and years

There is no source code change to `java.management` anymore inside this PR. 
They will be resolved with new bugs at 
[JDK-8327618](https://bugs.openjdk.org/browse/JDK-8327618) and 
[JDK-8328263](https://bugs.openjdk.org/browse/JDK-8328263). There are test 
changes in these areas in this PR to force them running with SM allowed. 
Ideally, these changes can be reverted when the 2 new bugs are resolved.

-

PR Comment: https://git.openjdk.org/jdk/pull/17472#issuecomment-2009779837

Re: RFR: 8328592: hprof tests fail with -XX:-CompactStrings

[Paul Hohensee]

On Wed, 20 Mar 2024 09:53:36 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

Are we guaranteed that non-compact string char component bytes are stored in 
little-endian order?

-

PR Review: https://git.openjdk.org/jdk/pull/18394#pullrequestreview-1949113961

Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v8]

[Weijun Wang]

> This code change adds an alternative implementation of user-based 
> authorization `Subject` APIs that doesn't depend on Security Manager APIs. 
> Depending on if the Security Manager is allowed, the methods store the 
> current subject differently. See the spec change in the `Subject.java` file 
> for details. When the Security Manager APIs are finally removed in a future 
> release, this new implementation will be only implementation for these 
> methods.
> 
> One major change in the new implementation is that `Subject.getSubject` 
> always throws an `UnsupportedOperationException` since it has an 
> `AccessControlContext` argument but the current subject is no longer 
> associated with an `AccessControlContext` object.
> 
> Now it's the time to migrate from the `getSubject` and `doAs` methods to 
> `current` and `callAs`. If the user application is simply calling 
> `getSubject(AccessController.getContext())`, then switching to `current()` 
> would work. If the `AccessControlContext` argument is retrieved from an 
> earlier `getContext()` call and the associated subject might be different 
> from that of the current `AccessControlContext`, then instead of storing the 
> previous `AccessControlContext` object and passing it into `getSubject` to 
> get the "previous" subject, the application should store the `current()` 
> return value directly.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  more allow and years

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17472/files
  - new: https://git.openjdk.org/jdk/pull/17472/files/dfa22af0..1e6a7982

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17472=07
 - incr: https://webrevs.openjdk.org/?repo=jdk=17472=06-07

  Stats: 9 lines in 6 files changed: 0 ins; 0 del; 9 mod
  Patch: https://git.openjdk.org/jdk/pull/17472.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17472/head:pull/17472

PR: https://git.openjdk.org/jdk/pull/17472

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v4]

[Martin Doerr]

On Wed, 20 Mar 2024 14:02:23 GMT, Richard Reingruber  wrote:

>> This pr changes  `JfrJvmtiAgent::retransform_classes()` and 
>> `jfr_set_enabled` to switch to `WXWrite` before transitioning to the vm.
>> 
>> Testing:
>> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java 
>> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
>> 
>> More tests are pending.
>
> Richard Reingruber has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains five additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite
>  - 2 more locations
>  - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite
>  - Set WXWrite at more locations
>  - Switch to WXWrite before entering the vm

Good catch!

-

Marked as reviewed by mdoerr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18238#pullrequestreview-1948978584

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v4]

[Richard Reingruber]

> This pr changes  `JfrJvmtiAgent::retransform_classes()` and `jfr_set_enabled` 
> to switch to `WXWrite` before transitioning to the vm.
> 
> Testing:
> make test TEST=jdk/jfr/event/runtime/TestClassLoadEvent.java 
> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
> make test TEST=compiler/intrinsics/klass/CastNullCheckDroppingsTest.java 
> TEST_VM_OPTS=-XX:+AssertWXAtThreadSync
> 
> More tests are pending.

Richard Reingruber has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains five additional 
commits since the last revision:

 - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite
 - 2 more locations
 - Merge branch 'master' into 8327990_jfr_enters_vm_without_wxwrite
 - Set WXWrite at more locations
 - Switch to WXWrite before entering the vm

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18238/files
  - new: https://git.openjdk.org/jdk/pull/18238/files/1b4d7606..8239638b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18238=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=18238=02-03

  Stats: 6136 lines in 243 files changed: 2379 ins; 2538 del; 1219 mod
  Patch: https://git.openjdk.org/jdk/pull/18238.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18238/head:pull/18238

PR: https://git.openjdk.org/jdk/pull/18238

Re: RFR: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync [v3]

[Richard Reingruber]

On Tue, 19 Mar 2024 17:46:31 GMT, Tobias Holenstein  
wrote:

>> I've asked this myself (after making the change).
>> Being in `WXWrite` mode would be wrong if the thread would execute 
>> dynamically generated code. There's not too much happening outside the scope 
>> of the `ThreadInVMfromNative` instances. I see jni calls 
>> (`GetObjectArrayElement`, `ExceptionOccurred`) and a jvmti call 
>> (`RetransformClasses`) but these are safe because the callees enter the vm 
>> right away. We even avoid switching to `WXWrite` and back there.
>> So I thought it would be ok to coarsen the WXMode switching.
>> But maybe it's still better to avoid any risk especially since there's 
>> likely no performance effect.
>
> Or could the  `ThreadInVMfromNative tvmfn(THREAD);` in 
> `check_exception_and_log` be move out to 
> `JfrJvmtiAgent::retransform_classes`?  And then only use one 
> `ThreadInVMfromNative` in `JfrJvmtiAgent::retransform_classes`

I think this would require hoisting the `ThreadInVMfromNative` out of the loop 
with the `check_exception_and_log` call but then the thread would be in 
`_thread_in_vm` when doing the `GetObjectArrayElement` jni call which would be 
wrong.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18238#discussion_r1532121487

RE: RFD: Remove Hotspot-internal MBeans from sun.management

[Kevin Walls]

Yes, I looked into it a little since you raised the previous issue, that looks 
like the right direction.
HotspotInternalMBean should be removed (an undocumented, unsupported and 
experimental interface)

HotspotInternalMBean creates: sun.management….
HotspotClassLoading
HotspotMemory
HotspotRuntime
HotspotThread
HotspotCompilation

These have a peculiar way of accessing that we do not document as far as I can 
see.

It would be good to look at whether anything in there is not adequately 
available by other MBeans, or jstat, or any other mechanism.

Thanks
Kevin


From: Mandy Chung 
Sent: 19 March 2024 16:48
To: Eirik Bjørsnøs ; serviceability-dev@openjdk.org
Cc: Volker Simonis ; Kevin Walls 
; Alan Bateman 
Subject: Re: RFD: Remove Hotspot-internal MBeans from sun.management


Would a PR to remove these APIs be welcome?

Good with me.

Mandy
On 3/19/24 9:41 AM, Eirik Bjørsnøs wrote:

Hi,

Last September, Volker shared the observation that we have Hotspot-internal 
MBeans in sun.management which are strongly encapsulated and not used 
internally by OpenJDK besides their unit tests:

https://www.mail-archive.com/core-libs-dev@openjdk.org/msg19878.html

A summary of the email thread:

Mandy pointed out:

We added these HotSpot internal MBeans in JDK 5 to expose the internal metrics. 
 Most of these internal metrics are exposed via jstat tool too.   We didn't 
receive much feedback regarding these HotSpot internal MBeans.Removing them 
is fine and good cleanup effort.

Alan made a similar point:

It's left over from experiments on exposing some internal metrics, I think 
during JDK 5. It's code that should probably have been removed a long time ago.

Kirk P raised a concern:

It would be a shame to lose these metrics because many of them have been very
useful over time and some would be even more useful with some modifications.

To which Mandy responded:

What we're referring to is to remove sun.management.Hotspot*, the internal 
MBeans which are never exposed and registered in the platform MBeanServer.   
The internal metrics in HotSpot VM should be retained as they are exposed 
through other ways like jstat, GC logs, etc.

The email thread seems to have ended here without further action taken.

My interpretation of the above is that we have a consensus that these 
Hotspot-internal MBeans can be removed. Since I was not part of the initial 
discussion and some time has passed, I'd like some confirmation that my 
interpretation is correct.

Would a PR to remove these APIs be welcome?

(This would remove HotspotInternalMBean, HotspotMemoryMBean, 
HotspotRuntimeMBean, HotspotThreadMBean, with associated implementation, 
factory methods, tests and probably also some native code in libmanagement. 
Details can be discussed in a PR)

Cheers,
Eirik.

Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v2]

[Erik Gahlin]

On Tue, 19 Mar 2024 17:58:46 GMT, Bill Huang  wrote:

>> This task addresses an essential aspect of our testing infrastructure: the 
>> proper handling and cleanup of temporary files and socket files created 
>> during test execution. The motivation behind these changes is to prevent the 
>> accumulation of unnecessary files in the default temporary directory, which 
>> can affect the system's storage and potentially influence subsequent test 
>> runs.
>> 
>> Our review identified that several tests create temporary files or socket 
>> files without ensuring their removal post-execution. 
>> - Direct calls to java.io.File.createTempFile and 
>> java.nio.file.Files.createTempFile without adequate cleanup.
>> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not 
>> explicitly removing socket files post-use.
>
> Bill Huang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Implemented review comments

Speaking for JFR, we should probably just create a normal file, possibly with a 
filename to indicate subtest and iteration. That said, test changes for JFR 
looks fine, and fixing the filename is outside the scope of this bug.

-

PR Comment: https://git.openjdk.org/jdk/pull/18352#issuecomment-2009218768

RFR: 8328592: hprof tests fail with -XX:-CompactStrings

[Aleksey Shipilev]

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

-

Commit messages:
 - Fix

Changes: https://git.openjdk.org/jdk/pull/18394/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=18394=00
  Issue: https://bugs.openjdk.org/browse/JDK-8328592
  Stats: 19 lines in 2 files changed: 13 ins; 1 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/18394.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18394/head:pull/18394

PR: https://git.openjdk.org/jdk/pull/18394

Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v8]

[Andreas Steiner]

On Tue, 19 Mar 2024 20:15:18 GMT, Chris Plummer  wrote:

>> So should I also use  HeapDumpGzipLevel  the same way as HeapDumpPath ? Tehn 
>> we have to change the text in globals.hpp for HeapDumpGzipLevel as well 
>> because it mentions only the HeapDumpOnOutOfmemoryError case and not the 
>> jcmd case .
>
> I actually mentioned  HeapDumpGzipLevel as another reason not to make this 
> change. I'm still not seeing it's value relative to the documentation 
> headaches it causes.
> 
> @ansteiner commented that:
> 
>> This is really helpful from a support point of view.
> 
> I'd like to understand how. It seems to me that the person using the jcmd has 
> a lot more interest in where the file ends up than the person launching the 
> JVM, and can always specify the location with the jcmd. It seems odd to me 
> that the jcmd user would want to rely on HeapDumpPath when they can just 
> specify the path with the jcmd and know for sure where the file is going to 
> end up.

In a cloud environment using containers, the HeapDumpPath is automatically set 
to a file system service to persist the heapdump. However, if a support 
engineer or DevOps detects high or increasing memory usage and wants to trigger 
a heapdump via jcmd, they would need to specify the filename. This requires 
retrieving the set HeapDumpPath from the app environment and copying it to the 
jcmd to use the persistent file system. This change can help avoid the need for 
an additional copy and paste step, which is prone to errors.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18190#discussion_r1531715298

Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v3]

[Serguei Spitsyn]

> The `JvmtiHandshake` and `JvmtiUnitedHandshakeClosure` classes were 
> introduced in the JDK 22 to unify/simplify the JVM TI functions supporting 
> implementation of the virtual threads. This enhancement is to refactor JVM TI 
> functions `GetOwnedMonitorInfo` and `GetOwnedMonitorStackDepthInfo` on the 
> base of `JvmtiHandshake and `JvmtiUnitedHandshakeClosure` classes.
> 
> Testing:
>  - Ran mach5 tiers 1-6

Serguei Spitsyn has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains four additional 
commits since the last revision:

 - Merge
 - review: correct one comment
 - added a couple of more fields to the JvmtiUnifiedHaandshakeClosure
 - 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18332/files
  - new: https://git.openjdk.org/jdk/pull/18332/files/78b7c894..ebbf0a0f

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18332=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=18332=01-02

  Stats: 294199 lines in 471 files changed: 6301 ins; 5813 del; 282085 mod
  Patch: https://git.openjdk.org/jdk/pull/18332.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18332/head:pull/18332

PR: https://git.openjdk.org/jdk/pull/18332

Re: RFR: 8328285: GetOwnedMonitorInfo functions should use JvmtiHandshake [v2]

[Serguei Spitsyn]

On Wed, 20 Mar 2024 02:10:28 GMT, Patricio Chilano Mateo 
 wrote:

>> Serguei Spitsyn has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   review: correct one comment
>
> src/hotspot/share/prims/jvmtiEnv.cpp line 1368:
> 
>> 1366:   if (java_thread != nullptr) {
>> 1367: Handle thread_handle(calling_thread, thread_oop);
>> 1368: EscapeBarrier eb(true, calling_thread, java_thread);
> 
> I see that now we will execute the EscapeBarrier code for the vthread case 
> too. We actually had to disable that for virtual threads since it doesn't 
> work (8285739 and 8305625). But we only addressed the case when targeting all 
> threads and not the single thread one. We would need to add the same check in 
> EscapeBarrier::deoptimize_objects(int d1, int d2) to skip a thread with 
> mounted continuation.

Good suggestion, thanks!
Would the following fix work ? :


git diff
diff --git a/src/hotspot/share/runtime/escapeBarrier.cpp 
b/src/hotspot/share/runtime/escapeBarrier.cpp
index bc01d900285..1b6d57644dc 100644
--- a/src/hotspot/share/runtime/escapeBarrier.cpp
+++ b/src/hotspot/share/runtime/escapeBarrier.cpp
@@ -75,7 +75,7 @@ bool EscapeBarrier::deoptimize_objects(int d1, int d2) {
 // These frames are about to be removed. We must not interfere with that 
and signal failure.
 return false;
   }
-  if (deoptee_thread()->has_last_Java_frame()) {
+  if (deoptee_thread()->has_last_Java_frame() && 
deoptee_thread()->last_continuation() == nullptr) {
 assert(calling_thread() == Thread::current(), "should be");
 KeepStackGCProcessedMark ksgcpm(deoptee_thread());
 ResourceMark rm(calling_thread());


BTW, it seems the `PopFrame` and the `ForceEarlyReturn` are broken the 
same way and, I hope, the fix above should solve the issue.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18332#discussion_r1531653123