search

eclipse-openj9 / openj9

Eclipse OpenJ9: A Java Virtual Machine for OpenJDK that's optimized for small footprint, fast start-up, and high throughput. Builds on Eclipse OMR (https://github.com/eclipse/omr) and combines with the Extensions for OpenJDK for OpenJ9 repo.
Other
3.28k stars 722 forks source link

Loom: DDR support #15181

Closed tajila closed 1 year ago

tajila commented 2 years ago

Depends on https://github.com/eclipse-openj9/openj9/issues/15177

Possibly add a new !virtualthreads command to list all virtual threads.

Or perhaps add an option to !threads [virtual | platform | all]

Add !continuationstack [j9vmcontinuation] to dump stacktrace, similar to !stack. We will have a stackslots and jitstack variant as well.

GC check in DDR will need to be updated to scan Continuation stacks as well

tajila commented 2 years ago

FYI @keithc-ca

fengxue-IS commented 2 years ago

@thallium This is the issue for tracking progress on DDR work for Loom

thallium commented 2 years ago

Added virtual thread to !threads in #15679 sample output:

!continuationstack 0x00007fe3582424e0 !j9vmcontinuation 0x00007fe3582424e0 !j9object 0x000000070637E4A8 (Continuation) !j9object 0x000000070637DED0 (VThread) - duke

for multi virtualthread core file:

!continuationstack 0x00007f5d2c160920 !j9vmcontinuation 0x00007f5d2c160920 !j9object 0x0000000706391058 (Continuation) !j9object 0x0000000706390A80 (VThread) - 
!continuationstack 0x00007f5d2c241f60 !j9vmcontinuation 0x00007f5d2c241f60 !j9object 0x00000007063B4A20 (Continuation) !j9object 0x00000007063B4980 (VThread) - 
!continuationstack 0x00007f5d2c24ebd0 !j9vmcontinuation 0x00007f5d2c24ebd0 !j9object 0x00000007063B4B68 (Continuation) !j9object 0x00000007063B4AC8 (VThread) - 
!continuationstack 0x00007f5d2c24ecc0 !j9vmcontinuation 0x00007f5d2c24ecc0 !j9object 0x00000007063B4CB0 (Continuation) !j9object 0x00000007063B4C10 (VThread) - 
!continuationstack 0x00007f5d2c24edb0 !j9vmcontinuation 0x00007f5d2c24edb0 !j9object 0x00000007063B4DF8 (Continuation) !j9object 0x00000007063B4D58 (VThread) - 
!continuationstack 0x00007f5d2c24eea0 !j9vmcontinuation 0x00007f5d2c24eea0 !j9object 0x00000007063B7A68 (Continuation) !j9object 0x00000007063B79C0 (VThread) -
fengxue-IS commented 2 years ago

Loom DDR support for scanning Continuation stack:

in order to correctly scan the java stack that is associated with a Continuation object, the JVM currently creates a J9VMThread structure and copy the fields from J9VMContinuation and uses that thread to perform the stack walk operation. To achieve similar behaviour in DDR, we purpose to create a subclass of J9VMThreadPointer.class that accepts a J9VMContinuation as base and will override all the getter methods which are used during StackWalk to return the data from Continuation instead. one of the key difficulty is to create the correct ELS (EntryLocalStorage) structure for the Thread. the necessary info on JIT GP register data (J9JITGPRSpillArea) is stored in J9VMContinuation, we would have to find a way of writing a ELS structure in the corefile's memory space. Purposed solution is to allocate a stub ELS struct and attach it to J9JavaVM during VM startup which can be accessed and used during DDR as it already exist in the corefile memoryspace. We would have to figure out a way of writing data into the structure rather than reading from it.

keithc-ca commented 2 years ago

J9VMThreadPointer is a generated class; perhaps it would make more sense to expand J9ThreadHelper to handle the new use-cases.

fengxue-IS commented 2 years ago

J9VMThreadPointer is a generated class; perhaps it would make more sense to expand J9ThreadHelper to handle the new use-cases.

Currently in DDR we use StackWalker.walkStackFrames(walkState, sp, arg0EA, pc, literals, entryLocalStorage) which accepts a J9VMThreadPointer as an argument part of walkState. I am not sure how expanding J9ThreadHelper would be able to handle such case.

The way that a Continuation is mounted to vmThread is

J9VMContinuation *continuation;

vmThread->arg0EA = continuation->arg0EA;
vmThread->bytecodes = continuation->bytecodes;
vmThread->sp = continuation->sp;
vmThread->pc = continuation->pc;
vmThread->literals = continuation->literals;
vmThread->stackOverflowMark = continuation->stackOverflowMark;
vmThread->stackOverflowMark2 = continuation->stackOverflowMark2;
vmThread->stackObject = continuation->stackObject;
vmThread->j2iFrame = NULL;

J9VMEntryLocalStorage els;
els.oldEntryLocalStorage = continuation.oldEntryLocalStorage;
els.gitGlobalStorageBase = (UDATA*)&continuation->jitGPRs;

vmThread->entryLocalStorage = ⪕

So the only options I see is to either extend J9VMThreadPointer to take a J9VMContinuation and override getters to return value based on the above mapping, or to have a stub J9VMThread that we will overwrite with fields from continuation and use that thread struct to do the stackwalk

keithc-ca commented 2 years ago

In the context of examining a core file, the carrier thread has already been updated as above or it hasn't (we may need to be cautious about intermediate states). As such, J9ThreadPointer should (except for those intermediate states) have appropriate fields, so StackWalker.walkStackFrames() should just do the right thing.

If on the other hand, you want to also present the stacks of continuations (with no current carrier thread), then I think the right place to start is by implementing a new IStackWalker for those.

keithc-ca commented 2 years ago

Another reason why extending J9VMThreadPointer is problematic: such classes represent a read-only view of the referenced structure.

fengxue-IS commented 2 years ago

In the context of examining a core file, the carrier thread has already been updated as above or it hasn't (we may need to be cautious about intermediate states). As such, J9ThreadPointer should (except for those intermediate states) have appropriate fields, so StackWalker.walkStackFrames() should just do the right thing.

Right.

If on the other hand, you want to also present the stacks of continuations (with no current carrier thread), then I think the right place to start is by implementing a new IStackWalker for those.

The reason I purposed this design is to avoid the need to implement a new IStackWalker as we will be duplicating all of the existing code except for those that access values from walkState.walkThread, I guess if there isn't a easy workaround to modify data/getter of a J9VMThreadPointer, then we would have to take this path.

thallium commented 2 years ago

Is it possible to rewrite StackWalker.walkStackFrames() so it will only use the arguments instead of reading from walkState.walkThread?

keithc-ca commented 2 years ago

Is it possible to rewrite StackWalker.walkStackFrames() so it will only use the arguments instead of reading from walkState.walkThread?

That seems a reasonable way towards having a stackwalker for continuations: I suggest com.ibm.j9ddr.vm29.j9.stackwalker.WalkState should be modified by removing walkThread and replacing it with a set of field capturing the necessary information for StackWalker_29_V0.

fengxue-IS commented 2 years ago

That seems a reasonable way towards having a stackwalker for continuations: I suggest com.ibm.j9ddr.vm29.j9.stackwalker.WalkState should be modified by removing walkThread and replacing it with a set of field capturing the necessary information for StackWalker_29_V0.

This sounds like a good starting point, digging through the code most uses of walkState.walkThread is for retrieving javaVM, so that can be replaced. To avoid modifying existing callers to StackWalker.walkStackFrames(), I suggest we add new fields to WalkState and not remove walkThread. then initialize the new fields based on the walkThread if they have not been initialized.

plus we would still need to find a way of constructing a readable entryLocalStorage based on the jitGPR data for the new design with direct interpreter transition, that is something I still feel should be done using a subclass of the structure

keithc-ca commented 2 years ago

I suggest we add new fields to WalkState and not remove walkThread. then initialize the new fields based on the walkThread if they have not been initialized.

I suggest this is a dangerous path: we won't get any help from the compiler recognizing those places that should not be using walkThread leading to unwanted behavior.

fengxue-IS commented 2 years ago

@thallium For a prototype: add following fields to WalkState

J9JavaVMPointer javaVM;
long threadAddress;
IOSThread osThread;
long privateFlags;

For every place that calls StackWalker.walkStackFrames() initialize the following fields using info from thread/continuation before calling it

walkState.javaVM
walkState.threadAddress
walkState.osThread
walkState.privateFlags
walkState.arg0EA    
walkState.pc
walkState.pcAddress
walkState.walkSP
walkState.literals
walkState.walkedEntryLocalStorage
walkState.j2iFrame
walkState.decompilationStack

you will also have to update the walkStackFrames code to remove dependency on walkThread in the code

you will also need to update all methods called within StackWalker.walkStackFrames that uses walkThread to either use those field directly, or for callback functions pass the javaVM instead

In the prototype, just use a subclass of J9VMEntryLocalStoragePointer

class J9VMEntryLocalStoragePointerWrapper extends J9VMEntryLocalStoragePointer {
    public J9VMEntryLocalStoragePointer oldELS;
    public UDATAPointer jitGPRs;

    @Override
    public J9VMEntryLocalStoragePointer oldEntryLocalStorage() {
        return oldELS;
    }
    @Override
    public UDATAPointer jitGlobalStorageBase() {
        return jitGPRs;
    }
}

you will also need to add a stub struct of J9VMEntryLocalStorage to the javaVM so you can reference it.

J9JavaVMPointer vm = ...;
J9VMEntryLocalStoragePointer wrapperELS = J9VMEntryLocalStoragePointerWrapper.cast(vm.entryLocalStorageEA());
wrapperELS.oldELS = Continuation.oldELS();
wrapperELS.jitGPRs = UDATAPointer.cast(Continuation.jitGPRsEA());
thallium commented 2 years ago

I think privateFlags could be UDATA to better work with existing code?

keithc-ca commented 2 years ago

I think privateFlags could be UDATA to better work with existing code?

Agreed. That will make changes to StackWalker more straight-forward (e.g. we can continue to use anyBitsIn(), etc.).

thallium commented 2 years ago

@keithc-ca We are planning to add two more overloads for walkStackFrame, one for J9VMThreadPointer - walkStackFrame(WalkState walkState, J9VMThreadPointer walkThread), and one for J9VMContinuationPointer - walkStackFrame(WalkState walkState, long address)(since J9VMContinuationPointer is not available, we use address instead and do the reflection in the function). Both overloads will eventually call the existing walkStackFrame.

keithc-ca commented 2 years ago

To me, it either makes sense to replace the one method with two, or add one for continuations: why would we want three in total?

thallium commented 2 years ago

Actually there are two existing versions:

public StackWalkResult walkStackFrames(WalkState walkState)
public StackWalkResult walkStackFrames(WalkState walkState, UDATAPointer sp, UDATAPointer arg0ea,  U8Pointer pc, J9MethodPointer literals, J9VMEntryLocalStoragePointer entryLocalStorage)

and the first one calls the second one. I guess we can get rid of the second one since we can fill the fields before calling the function, but the first is still needed in case people supply their own sp, arg0ea, etc.

thallium commented 2 years ago

Sample output for continuationstack command:

> !continuationstack 0x00007fe78c0f9600                                                                                                                                                                                                                                                     
<7fe78c0f9600>  !j9method 0x000000000006F218   jdk/internal/vm/Continuation.yieldImpl(Z)Z                                                                                                                                                                                                   
<7fe78c0f9600>  !j9method 0x000000000006F098   jdk/internal/vm/Continuation.yield(Ljdk/internal/vm/ContinuationScope;)Z                                                                                                                                                                     
<7fe78c0f9600>  !j9method 0x00000000001EA350   java/lang/VirtualThread.yieldContinuation()Z                                                                                                                                                                                                 
<7fe78c0f9600>  !j9method 0x00000000001EA430   java/lang/VirtualThread.parkNanos(J)V                                                                                                                                                                                                        
<7fe78c0f9600>  !j9method 0x00000000001EA510   java/lang/VirtualThread.doSleepNanos(J)V                                                                                                                                                                                                     
<7fe78c0f9600>  !j9method 0x00000000001EA4F0   java/lang/VirtualThread.sleepNanos(J)V                                                                                                                                                                                                       
<7fe78c0f9600>  !j9method 0x0000000000049788   java/lang/Thread.sleep(Ljava/time/Duration;)V                                                                                                                                                                                                
<7fe78c0f9600>  !j9method 0x00000000001C6920   VTTest2.lambda$main$0(I)Ljava/lang/Integer;                                                                                                                                                                                                  
<7fe78c0f9600>  !j9method 0x00000000001C6DE8   VTTest2$$Lambda$28/0x000000008c211470.call()Ljava/lang/Object;                                                                                                                                                                               
<7fe78c0f9600>  !j9method 0x00000000001D3CB8   java/util/concurrent/ThreadPerTaskExecutor$ThreadBoundFuture.run()V                                                                                                                                                                          
<7fe78c0f9600>  !j9method 0x00000000001EA2F0   java/lang/VirtualThread.run(Ljava/lang/Runnable;)V                                                                                                                                                                                           
<7fe78c0f9600>  !j9method 0x00000000001F2EA0   java/lang/VirtualThread$VThreadContinuation.lambda$new$0(Ljava/lang/VirtualThread;Ljava/lang/Runnable;)V                                                                                                                                     
<7fe78c0f9600>  !j9method 0x00000000001F12C8   java/lang/VirtualThread$VThreadContinuation$$Lambda$34/0x0000000000000000.run()V                                                                                                                                                             
<7fe78c0f9600>  !j9method 0x000000000006F058   jdk/internal/vm/Continuation.execute(Ljdk/internal/vm/Continuation;)V                                                                                                                                                                        
<7fe78c0f9600>                          JNI call-in frame                                                                                                                                                                                                                                   
<7fe78c0f9600>                          Native method frame
fengxue-IS commented 2 years ago

DDR support for Project Loom (Revised)

Goals

  1. List all virtual threads.
  2. Get the stack and stackslots for a virtual thread.

Approach

Goal 1: List virtual threads

Command syntax:

Option 1: !virtualthreads
Option 2: !vthreads (can be an alias)

Output syntax:

!continuationstack 0x%016x !j9vmcontinuation 0x%016x !j9object %s (Continuation) !j9object %s (VThread) - %s

!continuationstack 0x%016x: output similar to the existing !stack 0x<thread> command
!j9vmcontinuation 0x%016x: outputs the elements of the J9VMContinuation struct
!j9object %s (Continuation): outputs the Continuation object
!j9object %s (VThread): outputs the VirtualThread object
%s: the last string contains the virtual thread name

This command tries to match the existing !thread command's output syntax:

!stack 0x%08x !j9vmthread 0x%08x !j9thread 0x%08x tid 0x%x (%d) // (%s)

!stack 0x%08x: Java stack of the VM thread
!j9vmthread 0x%08x: outputs the elements of the J9VMThread struct
!j9thread 0x%08x: outputs the elements of the J9Thread struct
tid 0x%x (%d): outputs the thread-ID for the native (OS) thread
// (%s): the last string contains the VM thread's name

Goal 2: Stack and stackslots for a virtual thread

Command syntax:

Option 1:
  !continuationstack 0x<thread>
  !continuationstackslots 0x<thread>

Option 2:
  !vstack 0x<thread>
  !vstackslots 0x<thread>

Output syntax: Similar to the existing !stack and !stackslots commands.

FYI @keithc-ca @thallium

dmitripivkine commented 2 years ago

just in case you missed it: !threads has more functionality:

> !threads help
!threads            -- list all threads in the VM
!threads stack      -- list stacks for all threads in the VM
!threads stackslots -- list stackslots for all threads in the VM
!threads flags      -- print the public and private flags field for each thread
!threads debugEventData -- print the debugEventData fields for each thread
!threads search     -- find a thread by thread id
!threads monitors     -- list all monitors owned by each thread
!threads trace     -- show UTE thread information

I think you can investigate which subcommands would be useful for !vthreads implementation

fengxue-IS commented 2 years ago

For continuationstack command (#15779)

instead of using J9VMEntryLocalStoragePointerWrapper to extend the structure, new design is to have an interface which encapsulates the ELS data and provide identical api for both thread ELS and Continuation ELS data.

draft impl as follow:

public class ThreadELS implements EntryLocalStorage { private J9VMEntryLocalStoragePointer target; public ThreadELS(J9VMEntryLocalStoragePointer target) { this.target = target; }

public EntryLocalStorage oldEntryLocalStorage() {
    return new ThreadELS(target.oldEntryLocalStorage());
}

...

}

public class ContinuationELS implements EntryLocalStorage { private J9VMContinuationPointer target; public ContinuationELS(J9VMContinuationPointer target) { this.target = target; }

public EntryLocalStorage oldEntryLocalStorage() {
    return new ThreadELS(target.oldEntryLocalStorage());
}

public UDATAPointer jitGlobalStorageBase() {
    return UDATAPointer.cast((long)target.jitGPRs().getAddress());
}

public UDATAPointer jitFPRegisterStorageBase() {
    throw new UnsupportedOperationException();
}

...

}



Todo(s):
1. Convert all use of `J9VMEntryLocalStoragePointer` type in the StackWalker/JITStackWalker class to the interface type
2. Update call locations where `J9VMEntryLocalStoragePointer` is passed to stack walker and convert to matching impl class

@thallium @babsingh @keithc-ca Any suggestions/comments on this design? Any suggestion for naming as well?
babsingh commented 2 years ago

re https://github.com/eclipse-openj9/openj9/issues/15181#issuecomment-1277942198:

My preference:

// This syntax is short and easy to type. The description can specify verbose details.
Option 2: !vthreads

// Although long, this syntax will support both virtual threads and structured concurrency.
Option 1:
  !continuationstack 0x<thread>
  !continuationstackslots 0x<thread>
babsingh commented 2 years ago

re https://github.com/eclipse-openj9/openj9/issues/15181#issuecomment-1277951518:

All our useful. The first three are critical, which are currently being implemented. Others are a future TODO. Consideration: there can be a million virtual threads. Printing any information (stack|stackslots|other) for a million virtual threads may be exhaustive and counter-productive. 

// These will be supported by !vthreads, !continuationstack (implicit) and !continuationstackslots (implicit)
!threads            -- list all threads in the VM
!threads stack      -- list stacks for all threads in the VM
!threads stackslots -- list stackslots for all threads in the VM

// Can only be supported if a virtual thread is mounted.
!threads flags      -- print the public and private flags field for each thread
!threads monitors     -- list all monitors owned by each thread
!threads debugEventData -- print the debugEventData fields for each thread

// Can support it but the search will be based on the virtual thread ID
!threads search     -- find a thread by thread id

// Difficult to support since virtual threads can be mounted to different J9VMThread's during its life-cycle
!threads trace     -- show UTE thread information
babsingh commented 2 years ago

re https://github.com/eclipse-openj9/openj9/issues/15181#issuecomment-1278033949:

fengxue-IS commented 2 years ago
  • ELS -> EntryLocalStorage. Instead of the abbreviation, a more verbose naming is better for readability.

+1 (updated comment with new name)

  • Can default methods be used in the interface to avoid duplication?

Yes to some degree, we can make the target an interface field that is generic then if the method is defined in the generic type, default method can be used. (ie. only getAddress/getHexAddress/notNull)

keithc-ca commented 2 years ago

I think neither ELS or EntryLocalStorage is the right name for that interface. It isn't "entry local storage", but rather, a context that can provide EntryLocalStorage and other things.

fengxue-IS commented 2 years ago

I think neither ELS or EntryLocalStorage is the right name for that interface. It isn't "entry local storage", but rather, a context that can provide EntryLocalStorage and other things.

EntryLocalStorageAccessor maybe? or do you have any suggestion on this?

tajila commented 1 year ago

Work is complete