Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add asserts and tracepoints in JVM_VirtualThread* functions #16325

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

babsingh
Copy link
Contributor

@babsingh babsingh commented Nov 15, 2022

The asserts verify that a virtual thread is only

  • added once to the list; and
  • removed IFF it was added to the list.

The previous and next fields of the virtual thread object are set to
null when it is removed from the list.

Tracepoints output important details to triage failures.

Related: #16249
Related: #16259

Signed-off-by: Babneet Singh sbabneet@ca.ibm.com

@babsingh
Copy link
Contributor Author

fyi @fengxue-IS @tajila

@babsingh
Copy link
Contributor Author

Test case

Author: @tajila

import java.time.Duration;
import java.time.Instant;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.LockSupport;
import java.util.HashMap;
import java.util.stream.IntStream;

public class PinALot {

    static final Object lock = new Object();

    public static void main(String[] args) throws Exception {
        int iterations = 100_000_000;
        if (args.length > 0) {
            iterations = Integer.parseInt(args[0]);
        }
        final int ITERATIONS = iterations;

        AtomicInteger count = new AtomicInteger();

        Thread thread = Thread.ofVirtual().start(() -> {
            synchronized (lock) {
                while (count.incrementAndGet() < ITERATIONS) {
                        IntStream.range(0, 6).forEach(i -> {
                                final HashMap list = new HashMap();
                                                Thread t = Thread.ofVirtual().start(() -> {
                                                        for(int j=0; j < 100; j++) { list.put( new Object(), new Object());}
                                                        try {
                                                                Thread.sleep(1);
                                                        } catch (Throwable tr) {

                                                                tr.printStackTrace();
                                                        }
                                                });

                        });
                 LockSupport.parkNanos(1);
                }
            }
        });

        boolean terminated;
        do {
            terminated = thread.join(Duration.ofMillis(500));
            System.out.println(Instant.now() + " => " + count.get());
        } while (!terminated);

        int countValue = count.get();
        if (countValue != ITERATIONS) {
            throw new RuntimeException("count = " + countValue);
        }
    }
}

@babsingh
Copy link
Contributor Author

The above test case produces the following GC failure: #16249 (comment). With this PR, the GC failure is no longer seen. Now, the test consistently produces the Invalid JIT return address failure: #16249 (comment).

fyi @dmitripivkine @nbhuiyan

@babsingh
Copy link
Contributor Author

* already been added to the list. Only add to the list if the previous and
* next elements in the list are null.
*/
if ((NULL == threadPrev) && (NULL == threadNext)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good.

But is this check actually needed? there is only a single location where JVM_VirtualThreadMountEnd(true) can be called from during the entire scope of a virtualthread (https://github.com/ibmruntimes/openj9-openjdk-jdk19/blob/openj9/src/java.base/share/classes/java/lang/VirtualThread.java#L282)

I don't really see how this can can occur in the code. Rather than bypassing the issue with this check, I would prefer to have an assert here and find out how a virtual thread can be firstMount more than once before adding this check.

Adding a trace point in JVM_VirtualThreadMountEnd with (vmthread, carrierObject, vthreadObject firstMount) would help to identify if there are any issues.

same for the unmount case with the only unmountBegin(true) called at https://github.com/ibmruntimes/openj9-openjdk-jdk19/blob/openj9/src/java.base/share/classes/java/lang/VirtualThread.java#L310

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is only a single location where JVM_VirtualThreadMountEnd(true) can be called from during the entire scope of a virtualthread

Can JVM_VirtualThreadMountEnd(true) be invoked multiple times?

@fengxue-IS I have converted the null-checks to asserts to verify your hypothesis. I am unable to reproduce the GC issue on my end. Can you confirm locally on your machine that the GC issue is no longer seen with the previously mentioned test-case?

Copy link
Contributor

@fengxue-IS fengxue-IS Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can JVM_VirtualThreadMountEnd(true) be invoked multiple times?

No unless something went wrong in JCL that allowed a vthread to be mounted to multiple carriers

I will try this change locally, it is just java --enable-preview -Xint PinALot or does it need additional parameters?

Copy link
Contributor Author

@babsingh babsingh Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, no additional parameters are needed. Also, try without -Xint to verify that only the Invalid JIT return address failure is seen. Will squash and consolidate commits once the fix has been verified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I run this locally for about 2hr ~ with -Xint

2022-11-15T21:49:01.130230505Z => 14093355

have not seen the GC issue using test case provided in #16325 (comment)

runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
@tajila
Copy link
Contributor

tajila commented Nov 15, 2022

Can you also add some tracepoints for the mount/unmount methods (all 4).

We need to capture virtualthread->cont->vmref (as thread object address is not stable) and last/firstmount.

@babsingh babsingh changed the title Null-checks for liveVirtualThreadList Add asserts and tracepoints in JVM_VirtualThread* functions Nov 15, 2022
@babsingh
Copy link
Contributor Author

@fengxue-IS @tajila Tracepoints have been added, and the commits have been squashed.

@babsingh babsingh force-pushed the null_checks_vt_list branch 2 times, most recently from 89b49d0 to c4b9275 Compare November 15, 2022 21:18
The asserts verify that a virtual thread is only
- added once to the list; and
- removed IFF it was added to the list.

The previous and next fields of the virtual thread object are set to
null when it is removed from the list.

Tracepoints output important details to triage failures.

Related: eclipse-openj9#16249
Related: eclipse-openj9#16259

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@tajila
Copy link
Contributor

tajila commented Nov 15, 2022

jenkins compile win jdk19

@tajila
Copy link
Contributor

tajila commented Nov 15, 2022

jenkins test sanity amac jdk19

@tajila tajila merged commit e5ee930 into eclipse-openj9:master Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants