On Thu, 15 Aug 2024 04:11:47 GMT, David Holmes <dholmes@openjdk.org> wrote:

...

Greetings,

Explicitly pin a virtual thread before acquiring the JFR string pool monitor because migrating a carrier thread local event writer object onto another carrier thread is impossible.

During event commit, the thread is in a critical section because it has loaded a carrier thread local event writer object. For virtual threads, a contended monitor, such as a synchronized block, is a point where a thread could become unmounted.

A monitor guards the JFR string pool, but remounting a virtual thread onto another carrier is impossible because of the event writer.

Therefore, it's imperative to use explicit pin constructs to prevent unmounting at this location.

Testing: jdk_jfr

Thanks Markus

src/jdk.jfr/share/classes/jdk/jfr/internal/StringPool.java line 93:

...

91: private static long storeString(String s) { 92: try { 93: pinVirtualThread();

The pin should be outside the try so that the finally can only happen if pinning was succesful.

Good point, thanks. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20588#discussion_r1718199292

On Thu, 15 Aug 2024 09:46:04 GMT, Alan Bateman <alanb@openjdk.org> wrote:

...

Good point, thanks.

Yes, it should only unpin if pin completed successfully.

Please see updated changeset. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20588#discussion_r1718203932

On Thu, 15 Aug 2024 09:54:24 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:

...

Greetings,

Explicitly pin a virtual thread before acquiring the JFR string pool monitor because migrating a carrier thread local event writer object onto another carrier thread is impossible.

During event commit, the thread is in a critical section because it has loaded a carrier thread local event writer object. For virtual threads, a contended monitor, such as a synchronized block, is a point where a thread could become unmounted.

A monitor guards the JFR string pool, but remounting a virtual thread onto another carrier is impossible because of the event writer.

Therefore, it's imperative to use explicit pin constructs to prevent unmounting at this location.

Testing: jdk_jfr

Thanks Markus

Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision:

hoist pinVirtualThread

Marked as reviewed by alanb (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/20588#pullrequestreview-2240145153

On Thu, 15 Aug 2024 11:20:23 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:

...

Greetings,

Explicitly pin a virtual thread before acquiring the JFR string pool monitor because migrating a carrier thread local event writer object onto another carrier thread is impossible.

During event commit, the thread is in a critical section because it has loaded a carrier thread local event writer object. For virtual threads, a contended monitor, such as a synchronized block, is a point where a thread could become unmounted.

A monitor guards the JFR string pool, but remounting a virtual thread onto another carrier is impossible because of the event writer.

Therefore, it's imperative to use explicit pin constructs to prevent unmounting at this location.

Testing: jdk_jfr

Thanks Markus

Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision:

update test comment

Functionally seems fine. Could be optimised a bit. src/jdk.jfr/share/classes/jdk/jfr/internal/StringPool.java line 86:

84: 85: private static void unpinVirtualThread() { 86: if (Thread.currentThread().isVirtual() && ContinuationSupport.isSupported()) {

If you are at all concerned about overhead here then pin could return a boolean to indicate if the pin happened and oyu could then unpin just by checking that boolean and avoid doing the isVirtual and isSupported checks again. ------------- Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20588#pullrequestreview-2240268782 PR Review Comment: https://git.openjdk.org/jdk/pull/20588#discussion_r1718332618

On Thu, 15 Aug 2024 14:16:11 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:

...

src/jdk.jfr/share/classes/jdk/jfr/internal/StringPool.java line 86:

...

84: 85: private static void unpinVirtualThread() { 86: if (Thread.currentThread().isVirtual() && ContinuationSupport.isSupported()) {

If you are at all concerned about overhead here then pin could return a boolean to indicate if the pin happened and oyu could then unpin just by checking that boolean and avoid doing the isVirtual and isSupported checks again.

Would it be possible to create a boolean in the EventWriter that indicates if it is associated with a carrier thread or a normal thread (which can never be virtual) and then have two methods.

long l = this.carrierThread ? StringPool.addPinnedString(s) : StringPool.addString(s);

Thread.currentThread() has an intrinsic, and isVirtual is just a type check. ContinuationSupport.isSupported reads a static final so will disappear once compiled. The pattern we are using in other areas is for the pin to return a boolean (like David suggested). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20588#discussion_r1718489921

On Wed, 21 Aug 2024 14:21:39 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:

...

Thread.currentThread() has an intrinsic, and isVirtual is just a type check. ContinuationSupport.isSupported reads a static final so will disappear once compiled. The pattern we are using in other areas is for the pin to return a boolean (like David suggested).

I looked into this in more detail. The current suggestion:

mov r10,QWORD PTR [r15+0x388] ; _vthread OopHandle mov r10,QWORD PTR [r10] ; dereference OopHandle <<-- Thread.currentThread() intrinsic gives 2 instructions mov r11d,DWORD PTR [r10+0x8] ; InstanceKlass to r11 <-- isVirtual() mov r10d,r11d ; InstanceKlass to r10 mov r8,QWORD PTR [r10+0x40] ; Load slot in InstanceKlass primary supers array to r8 movabs r10,0x2d0481a8 ; InstanceKlass for {metadata('java/lang/BaseVirtualThread')} to r10 cmp r8,r10 ; compare if superklass is java/lang/BaseVirtualThread jne 0x0000018571e0baf9 ; 6 instructions for isVirtual() type check, 8 instructions in total

This gives a prologue of eight instructions.

For JFR, we already have much of this information resolved when loading up the EventWriter instance using the existing intrinsic getEventWriter(). Hence, we could extend that to mark the event writer with a field to say if pinning should be performed. This results in only a two instruction prologue:

test r8d,r8d ; pinVirtualThread? je 0x0000012580a0f6c9 ; 2 instructions for test

This is an x4 speedup, although slightly less because of an additional store instruction for loading the event writer.

Further, I looked into the Continuation.pin() and Continuation.unpin() methods. They are currently not intrinsics, but lend themselves well to intrinsification. I have created such intrinsics, and the results are quite good.

Continuation.pin() or Continuation.unpin() without intrinsics = 112 instructions each Continuation.pin() or Continuation.unpin() with intrinsics = 8 instructions each

This is an x14 speedup for virtual threads.

I plan to fix the event writer under this PR (to be updated) and file a separate tracking enhancement for the intrinsification of Continuation.pin() and Continuation.unpin(). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20588#discussion_r1725150866

On Wed, 21 Aug 2024 18:26:20 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:

...

Greetings,

Explicitly pin a virtual thread before acquiring the JFR string pool monitor because migrating a carrier thread local event writer object onto another carrier thread is impossible.

During event commit, the thread is in a critical section because it has loaded a carrier thread local event writer object. For virtual threads, a contended monitor, such as a synchronized block, is a point where a thread could become unmounted.

A monitor guards the JFR string pool, but remounting a virtual thread onto another carrier is impossible because of the event writer.

Therefore, it's imperative to use explicit pin constructs to prevent unmounting at this location.

Testing: jdk_jfr

Thanks Markus

Markus Grönlund has updated the pull request incrementally with five additional commits since the last revision:

- pin state conditional delivered using event writer object - Merge branch '8338417' of github.com:mgronlun/jdk into 8338417 - update test comment - hoist pinVirtualThread - 8338417

Marked as reviewed by alanb (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/20588#pullrequestreview-2256567425