On Tue, 16 May 2023 23:43:51 GMT, Patricio Chilano Mateo
<[email protected]> wrote:
>> The following patch fixes a bug introduced while refactoring the
>> VirtualThreadStart/End events. Specifically, the code to delete the
>> JvmtiThreadState of a terminating vthread was moved before we start the VTMS
>> transition. That allowed said code to run concurrently with
>> recompute_enabled() leading to different crashing modes. I wrote the
>> detailed sequence of events leading to the crash in the bug comments.
>>
>> To fix it I moved the cleanup code back after the call to
>> VTMS_unmount_begin(). Now, since the rebinding of the JvmtiThreadState to
>> that of the carrier has to be done after this cleanup code is executed,
>> otherwise we would delete the wrong JvmtiThreadState state, I had to add a
>> boolean argument to VTMS_unmount_begin() to differentiate the last unmount
>> call from the other ones. This is unfortunate since ideally
>> VTMS_unmount_begin() would be oblivious to these two cases as with
>> VTMS_mount_end() where we don't need to check if this is the first mount.
>> I looked for other ways to solve it instead of the extra boolean argument
>> but wasn't convinced. One way would be to have another
>> JvmtiExport::cleanup_thread() that would handle this case. Another way which
>> is very simple is to move the rebind_to_jvmti_thread_state_of() call to
>> VTMS_unmount_end() instead. But that means during the transition the
>> _jvmti_thread_state field of the carrier would be either null or that of the
>> vthread, unlike today which is always that of the carrier during the
>> transitions. I didn't want to change that behavior in this fix but I can
>> also explore that route.
>>
>> I tested the patch with the reproducer I attached to the bug, plus I also
>> run tiers1-3 in mach5.
>>
>> Thanks,
>> Patricio
>
> Patricio Chilano Mateo has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Serguei test comments
Marked as reviewed by lmesnik (Reviewer).
test/hotspot/jtreg/serviceability/jvmti/vthread/ThreadStateTest/ThreadStateTest.java
line 44:
> 42: static final int VTHREAD_COUNT = 64;
> 43:
> 44: private static native void SetSingleSteppingMode(boolean enable);
Could you please rename them to 's'etSingleSteppingMode and
's'etMonitorContendedMode.
test/hotspot/jtreg/serviceability/jvmti/vthread/ThreadStateTest/ThreadStateTest.java
line 58:
> 56:
> 57: while (tryCount-- > 0) {
> 58: ExecutorService scheduler = Executors.newFixedThreadPool(8);
Is it possible to configure pool using any of these parameters?
-Djdk.defaultScheduler.parallelism=
-Djdk.defaultScheduler.maxPoolSize=
test/hotspot/jtreg/serviceability/jvmti/vthread/ThreadStateTest/ThreadStateTest.java
line 93:
> 91:
> 92: public static void main(String[] args) throws Exception {
> 93: try {
You could just remove try/catch.
-------------
PR Review: https://git.openjdk.org/jdk/pull/13949#pullrequestreview-1429692613
PR Review Comment: https://git.openjdk.org/jdk/pull/13949#discussion_r1195858682
PR Review Comment: https://git.openjdk.org/jdk/pull/13949#discussion_r1195860865
PR Review Comment: https://git.openjdk.org/jdk/pull/13949#discussion_r1195859018
