Skip to content

Commit

Permalink
[GR-57537] Avoid recursive lock method for jdwp suspension and pin ob…
Browse files Browse the repository at this point in the history 
…jects while in suspended state.

PullRequest: graal/19127
  • Loading branch information
@javeleon
javeleon committed Oct 29, 2024
2 parents f897b31 + ebd01c6 commit 8a88c44
Show file tree
Hide file tree
Showing 5 changed files with 125 additions and 113 deletions.
35 changes: 29 additions & 6 deletions ...so/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/api/Ids.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,17 @@
*/
package com.oracle.truffle.espresso.jdwp.api;

import com.oracle.truffle.espresso.jdwp.impl.DebuggerController;

import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.function.Supplier;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import com.oracle.truffle.espresso.jdwp.impl.DebuggerController;

/**
* Class that keeps an ID representation of all entities when communicating with a debugger through
* JDWP. Each entity will be assigned a unique ID. Only weak references are kept for entities.
Expand All @@ -57,6 +59,10 @@ public final class Ids<T> {

private DebuggerController controller;

private List<Object> pinnedObjects = new ArrayList<>();

private volatile boolean pinningState = false;

@SuppressWarnings({"unchecked", "rawtypes"})
public Ids(T nullObject) {
this.nullObject = nullObject;
Expand Down Expand Up @@ -84,8 +90,7 @@ public long getIdAsLong(T object) {
}
}
// check the anonymous inner class map
if (object instanceof KlassRef) {
KlassRef klass = (KlassRef) object;
if (object instanceof KlassRef klass) {
Long id = innerClassIDMap.get(klass.getNameAsString());
if (id != null) {
// inject new klass under existing ID
Expand Down Expand Up @@ -155,13 +160,16 @@ private synchronized long generateUniqueId(T object) {
expandedArray[objects.length] = new WeakReference<>(object);
objects = expandedArray;
log(() -> "Generating new ID: " + id + " for object: " + object);
if (object instanceof KlassRef) {
KlassRef klass = (KlassRef) object;
if (object instanceof KlassRef klass) {
Matcher matcher = ANON_INNER_CLASS_PATTERN.matcher(klass.getNameAsString());
if (matcher.matches()) {
innerClassIDMap.put(klass.getNameAsString(), id);
}
}
// pin object when VM in suspended state
if (pinningState) {
pinnedObjects.add(object);
}
return id;
}

Expand Down Expand Up @@ -203,4 +211,19 @@ private void log(Supplier<String> supplier) {
controller.finest(supplier);
}
}

public synchronized void pinAll() {
pinningState = true;
for (WeakReference<T> object : objects) {
Object toPin = object.get();
if (toPin != null) {
pinnedObjects.add(toPin);
}
}
}

public synchronized void unpinAll() {
pinningState = false;
pinnedObjects.clear();
}
}
109 changes: 48 additions & 61 deletions ...e.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/DebuggerController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public final class DebuggerController implements ContextsListener {
private final Map<Object, SimpleLock> suspendLocks = Collections.synchronizedMap(new HashMap<>());
private final Map<Object, SuspendedInfo> suspendedInfos = Collections.synchronizedMap(new HashMap<>());
private final Map<Object, SteppingInfo> commandRequestIds = new HashMap<>();
private final Map<Object, ThreadJob<?>> threadJobs = new HashMap<>();
private final Map<Object, InvokeJob<?>> invokeJobs = new HashMap<>();
private final Map<Object, FieldBreakpointEvent> fieldBreakpointExpected = new HashMap<>();
private final Map<Object, MethodBreakpointEvent> methodBreakpointExpected = new HashMap<>();
private final Map<Breakpoint, BreakpointInfo> breakpointInfos = new HashMap<>();
Expand Down Expand Up @@ -176,6 +176,10 @@ public JDWPContext getContext() {
return context;
}

public Ids<Object> getIds() {
return ids;
}

public SuspendedInfo getSuspendedInfo(Object thread) {
return suspendedInfos.get(thread);
}
Expand Down Expand Up @@ -386,6 +390,7 @@ public Object[] getVisibleGuestThreads() {
}

void forceResumeAll() {
ids.unpinAll();
for (Object thread : getVisibleGuestThreads()) {
boolean resumed = false;
SimpleLock suspendLock = getSuspendLock(thread);
Expand All @@ -398,6 +403,7 @@ void forceResumeAll() {
}

public void resumeAll() {
ids.unpinAll();
for (Object thread : getVisibleGuestThreads()) {
SimpleLock suspendLock = getSuspendLock(thread);
synchronized (suspendLock) {
Expand Down Expand Up @@ -453,6 +459,9 @@ public void immediateSuspend(Object eventThread, byte suspendPolicy, Callable<Vo
suspend(thread);
}
}
// pin all objects when VM in suspended state
ids.pinAll();

// immediately suspend the event thread
suspend(eventThread, SuspendStrategy.EVENT_THREAD, Collections.singletonList(callBack), true);
break;
Expand All @@ -469,6 +478,8 @@ public void suspendAll() {
for (Object thread : getVisibleGuestThreads()) {
suspend(thread);
}
// pin all objects
ids.pinAll();
}

private synchronized SimpleLock getSuspendLock(Object thread) {
Expand All @@ -480,7 +491,7 @@ private synchronized SimpleLock getSuspendLock(Object thread) {
return lock;
}

private String getThreadName(Object thread) {
String getThreadName(Object thread) {
return getContext().getThreadName(thread);
}

Expand Down Expand Up @@ -631,19 +642,18 @@ public void suspend(Object thread, byte suspendPolicy, List<Callable<Void>> jobs
case SuspendStrategy.ALL:
fine(() -> "Suspend ALL");

Thread suspendThread = new Thread(new Runnable() {
@Override
public void run() {
// suspend other threads
for (Object activeThread : getVisibleGuestThreads()) {
if (activeThread != thread) {
fine(() -> "Request thread suspend for other thread: " + getThreadName(activeThread));
DebuggerController.this.suspend(activeThread);
}
Thread suspendThread = new Thread(() -> {
// suspend other threads
for (Object activeThread : getVisibleGuestThreads()) {
if (activeThread != thread) {
fine(() -> "Request thread suspend for other thread: " + getThreadName(activeThread));
DebuggerController.this.suspend(activeThread);
}
}
});
suspendThread.start();
// pin all objects
ids.pinAll();
suspendEventThread(thread, forceSuspend, jobs);
break;
}
Expand All @@ -661,83 +671,60 @@ private static void runJobs(List<Callable<Void>> jobs) {

private void suspendEventThread(Object thread, boolean forceSuspend, List<Callable<Void>> jobs) {
fine(() -> "Suspending event thread: " + getThreadName(thread) + " with new suspension count: " + threadSuspension.getSuspensionCount(thread));
lockThread(thread, forceSuspend, true, jobs);
lockThread(thread, forceSuspend, jobs);
}

private void lockThread(Object thread, boolean forceSuspend, boolean isFirstCall, List<Callable<Void>> jobs) {
private void lockThread(Object thread, boolean forceSuspend, List<Callable<Void>> jobs) {
SimpleLock lock = getSuspendLock(thread);
// in case a thread job is already posted on this thread
checkThreadJobsAndRun(thread, forceSuspend);
synchronized (lock) {
if (!forceSuspend && !threadSuspension.isHardSuspended(thread)) {
// thread was resumed from other command, so don't suspend now
return;
}

if (lock.isLocked()) {
threadSuspension.suspendThread(thread);
runJobs(jobs);
}
}
while (!Thread.currentThread().isInterrupted()) {
try {
if (lock.isLocked() && isFirstCall) {
threadSuspension.suspendThread(thread);
runJobs(jobs);
}
while (lock.isLocked()) {
fine(() -> "lock.wait() for thread: " + getThreadName(thread));
synchronized (lock) {
if (!lock.isLocked()) {
// released from other thread, so break loop
break;
}
// no reason to hold a hard suspension status, since now
// we have the actual suspension status and suspended information
threadSuspension.removeHardSuspendedThread(thread);
lock.wait();
fine(() -> "lock.wait() for thread: " + getThreadName(thread));
// Having the thread lock, we can check if an invoke job was posted outside of
// locking, and if so, we postpone blocking the thread until next time around.
if (!invokeJobs.containsKey(thread)) {
lock.wait();
}
}
} catch (InterruptedException e) {
// the thread was interrupted, so let it run dry
// make sure the interrupted flag is set though
Thread.currentThread().interrupt();
}
checkInvokeJobsAndRun(thread);
}

checkThreadJobsAndRun(thread, forceSuspend);
getGCPrevention().releaseActiveWhileSuspended(thread);
fine(() -> "lock wakeup for thread: " + getThreadName(thread));
}

private void checkThreadJobsAndRun(Object thread, boolean forceSuspend) {
if (threadJobs.containsKey(thread)) {
// re-acquire the thread lock after completing
// the job, to avoid the thread resuming.
SimpleLock suspendLock = getSuspendLock(thread);
synchronized (suspendLock) {
suspendLock.acquire();
}
// a thread job was posted on this thread
// only wake up to perform the job a go back to sleep
ThreadJob<?> job = threadJobs.remove(thread);
byte suspensionStrategy = job.getSuspensionStrategy();

if (suspensionStrategy == SuspendStrategy.ALL) {
Object[] allThreads = getVisibleGuestThreads();
// resume all threads during invocation of method to avoid potential deadlocks
for (Object activeThread : allThreads) {
if (activeThread != thread) {
resume(activeThread);
}
}
// perform the job on this thread
job.runJob();
// suspend all other threads after the invocation
for (Object activeThread : allThreads) {
if (activeThread != thread) {
suspend(activeThread);
}
}
} else {
job.runJob();
}
lockThread(thread, forceSuspend, false, Collections.emptyList());
private void checkInvokeJobsAndRun(Object thread) {
if (invokeJobs.containsKey(thread)) {
InvokeJob<?> job = invokeJobs.remove(thread);
job.runJob(this);
}
}

public void postJobForThread(ThreadJob<?> job) {
public void postInvokeJobForThread(InvokeJob<?> job) {
SimpleLock lock = getSuspendLock(job.getThread());
synchronized (lock) {
threadJobs.put(job.getThread(), job);
lock.release();
invokeJobs.put(job.getThread(), job);
lock.notifyAll();
}
}
Expand Down
12 changes: 0 additions & 12 deletions ....oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/GCPrevention.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,13 @@
*/
package com.oracle.truffle.espresso.jdwp.impl;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;

public final class GCPrevention {

// simply hold a strong reference to all objects for which
// GC should be disabled
private final HashSet<Object> prevent = new HashSet<>();
private final HashMap<Object, ArrayList<Object>> activeWhileSuspended = new HashMap<>();

public void disableGC(Object object) {
prevent.add(object);
Expand All @@ -45,13 +42,4 @@ public void clearAll() {
prevent.clear();
}

public synchronized void setActiveWhileSuspended(Object guestThread, Object obj) {
activeWhileSuspended.putIfAbsent(guestThread, new ArrayList<>());
activeWhileSuspended.get(guestThread).add(obj);
}

public synchronized void releaseActiveWhileSuspended(Object guestThread) {
activeWhileSuspended.remove(guestThread);
}

}
32 changes: 24 additions & 8 deletions ...truffle/espresso/jdwp/impl/ThreadJob.java → ...truffle/espresso/jdwp/impl/InvokeJob.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

import java.util.concurrent.Callable;

public final class ThreadJob<T> {
public final class InvokeJob<T> {

private final Object jobLock = new Object();
private final Object thread;
Expand All @@ -33,11 +33,11 @@ public final class ThreadJob<T> {
private boolean resultAvailable;
private JobResult<T> result;

public ThreadJob(Object guestThread, Callable<T> task) {
public InvokeJob(Object guestThread, Callable<T> task) {
this(guestThread, task, SuspendStrategy.EVENT_THREAD);
}

public ThreadJob(Object guestThread, Callable<T> task, byte suspensionStrategy) {
public InvokeJob(Object guestThread, Callable<T> task, byte suspensionStrategy) {
this.thread = guestThread;
this.callable = task;
this.suspensionStrategy = suspensionStrategy;
Expand All @@ -47,17 +47,33 @@ public Object getThread() {
return thread;
}

public byte getSuspensionStrategy() {
return suspensionStrategy;
}

public void runJob() {
public void runJob(DebuggerController controller) {
Object[] visibleGuestThreads = controller.getVisibleGuestThreads();
result = new JobResult<>();
try {
if (suspensionStrategy == SuspendStrategy.ALL) {
controller.getIds().unpinAll();
// resume all other threads during invocation of method to avoid potential deadlocks
for (Object activeThread : visibleGuestThreads) {
if (activeThread != thread) {
controller.resume(activeThread);
}
}
}
// perform the job on this thread
result.setResult(callable.call());
} catch (Throwable e) {
result.setException(e);
} finally {
if (suspensionStrategy == SuspendStrategy.ALL) {
controller.getIds().pinAll();
// suspend all other threads after the invocation
for (Object activeThread : visibleGuestThreads) {
if (activeThread != thread) {
controller.suspend(activeThread);
}
}
}
resultAvailable = true;
synchronized (jobLock) {
jobLock.notifyAll();
Expand Down
Loading

0 comments on commit 8a88c44

Please sign in to comment.