[GR-57537] Avoid recursive lock method for jdwp suspension and pin objects while in suspended state.

PullRequest: graal/19127

Author: javeleon

espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/api/Ids.java

@@ -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.
@@ -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;
@@ -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
@@ -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;
     }
 
@@ -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();
+    }
 }

espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/DebuggerController.java

@@ -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<>();
@@ -176,6 +176,10 @@ public JDWPContext getContext() {
         return context;
     }
 
+    public Ids<Object> getIds() {
+        return ids;
+    }
+
     public SuspendedInfo getSuspendedInfo(Object thread) {
         return suspendedInfos.get(thread);
     }
@@ -386,6 +390,7 @@ public Object[] getVisibleGuestThreads() {
     }
 
     void forceResumeAll() {
+        ids.unpinAll();
         for (Object thread : getVisibleGuestThreads()) {
             boolean resumed = false;
             SimpleLock suspendLock = getSuspendLock(thread);
@@ -398,6 +403,7 @@ void forceResumeAll() {
     }
 
     public void resumeAll() {
+        ids.unpinAll();
         for (Object thread : getVisibleGuestThreads()) {
             SimpleLock suspendLock = getSuspendLock(thread);
             synchronized (suspendLock) {
@@ -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;
@@ -469,6 +478,8 @@ public void suspendAll() {
         for (Object thread : getVisibleGuestThreads()) {
             suspend(thread);
         }
+        // pin all objects
+        ids.pinAll();
     }
 
     private synchronized SimpleLock getSuspendLock(Object thread) {
@@ -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);
     }
 
@@ -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;
         }
@@ -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();
         }
     }

espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/GCPrevention.java

@@ -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);
@@ -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);
-    }
-
 }

espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/ThreadJob.java renamed to espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/InvokeJob.java

@@ -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;
@@ -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;
@@ -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();