Merge PR #357: Make Safepoint usage more robust and increase test tim…

…e outs
smarr · Jun 17, 2020 · 8a9e09b · 8a9e09b
2 parents 703a9c2 + bf74fa4
commit 8a9e09b
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 69 deletions.
diff --git a/src/som/interpreter/objectstorage/ObjectTransitionSafepoint.java b/src/som/interpreter/objectstorage/ObjectTransitionSafepoint.java
@@ -95,11 +95,13 @@ public void checkAndPerformSafepoint() {
   public void transitionObject(final SObject obj) {
     waitForSafepointStart();
 
-    // Safepoint phase, used to update the object
-    // object is required to handle updates from multiple threads correctly
-    obj.updateLayoutToMatchClass();
-
-    phaser.finishSafepointAndAwaitCompletion();
+    try {
+      // Safepoint phase, used to update the object
+      // object is required to handle updates from multiple threads correctly
+      obj.updateLayoutToMatchClass();
+    } finally {
+      phaser.finishSafepointAndAwaitCompletion();
+    }
   }
 
   /**
@@ -113,11 +115,13 @@ public void writeUninitializedSlot(final SObject obj, final SlotDefinition slot,
       final Object value) {
     waitForSafepointStart();
 
-    // Safepoint phase, used to update the object
-    // object is required to handle updates from multiple threads correctly
-    obj.writeUninitializedSlot(slot, value);
-
-    phaser.finishSafepointAndAwaitCompletion();
+    try {
+      // Safepoint phase, used to update the object
+      // object is required to handle updates from multiple threads correctly
+      obj.writeUninitializedSlot(slot, value);
+    } finally {
+      phaser.finishSafepointAndAwaitCompletion();
+    }
   }
 
   /**
@@ -131,22 +135,26 @@ public void writeAndGeneralizeSlot(final SObject obj, final SlotDefinition slot,
       final Object value) {
     waitForSafepointStart();
 
-    // Safepoint phase, used to update the object
-    // object is required to handle updates from multiple threads correctly
-    obj.writeAndGeneralizeSlot(slot, value);
-
-    phaser.finishSafepointAndAwaitCompletion();
+    try {
+      // Safepoint phase, used to update the object
+      // object is required to handle updates from multiple threads correctly
+      obj.writeAndGeneralizeSlot(slot, value);
+    } finally {
+      phaser.finishSafepointAndAwaitCompletion();
+    }
   }
 
   public void ensureSlotAllocatedToAvoidDeadlock(final SObject obj,
       final SlotDefinition slot) {
     waitForSafepointStart();
 
-    // Safepoint phase, used to update the object
-    // object is required to handle updates from multiple threads correctly
-    obj.ensureSlotAllocatedToAvoidDeadlock(slot);
-
-    phaser.finishSafepointAndAwaitCompletion();
+    try {
+      // Safepoint phase, used to update the object
+      // object is required to handle updates from multiple threads correctly
+      obj.ensureSlotAllocatedToAvoidDeadlock(slot);
+    } finally {
+      phaser.finishSafepointAndAwaitCompletion();
+    }
   }
 
   void renewAssumption() {

diff --git a/src/som/interpreter/objectstorage/SafepointPhaser.java b/src/som/interpreter/objectstorage/SafepointPhaser.java
@@ -261,7 +261,7 @@ void arriveAtSafepointAndAwaitStart() {
 
   void finishSafepointAndAwaitCompletion() {
     int phase = arriveAndAwaitAdvance();
-    assert (phase & 1) == 0 : "Expect phase to be evem on completion of safepoint, but was "
+    assert (phase & 1) == 0 : "Expect phase to be even on completion of safepoint, but was "
         + phase;
   }
 

diff --git a/tests/java/som/interpreter/objectstorage/SafepointPhaserTest.java b/tests/java/som/interpreter/objectstorage/SafepointPhaserTest.java
@@ -64,91 +64,106 @@ public void testThreadsRegisterTriggerSafepointAndUnregister() throws Interrupte
 
     ParallelHelper.executeNTimesInParallel((final int id) -> {
 
-      ObjectTransitionSafepoint.INSTANCE.register();
-
       try {
-        barrier.await();
-      } catch (InterruptedException | BrokenBarrierException e) {
-        throw new RuntimeException(e);
-      }
+        ObjectTransitionSafepoint.INSTANCE.register();
 
-      ObjectTransitionSafepoint.INSTANCE.transitionObject(
-          new SMutableObject(instanceClass, factory, layout));
+        try {
+          barrier.await();
+        } catch (InterruptedException | BrokenBarrierException e) {
+          throw new RuntimeException(e);
+        }
 
-      ObjectTransitionSafepoint.INSTANCE.unregister();
+        ObjectTransitionSafepoint.INSTANCE.transitionObject(
+            new SMutableObject(instanceClass, factory, layout));
+      } finally {
+        ObjectTransitionSafepoint.INSTANCE.unregister();
+      }
       return null;
     });
   }
 
+  private void transitionObject() {
+    for (int i = 0; i < 1181; i += 1) {
+      ObjectTransitionSafepoint.INSTANCE.transitionObject(
+          new SMutableObject(instanceClass, factory, layout));
+    }
+  }
+
   @Test
   public void testSafepointStorm() throws InterruptedException {
     ObjectTransitionSafepoint.reset();
     ParallelHelper.executeNTimesInParallel((final int id) -> {
 
-      ObjectTransitionSafepoint.INSTANCE.register();
+      try {
+        ObjectTransitionSafepoint.INSTANCE.register();
 
-      for (int i = 0; i < 500_000; i += 1) {
-        ObjectTransitionSafepoint.INSTANCE.transitionObject(
-            new SMutableObject(instanceClass, factory, layout));
+        for (int i = 0; i < 1181; i += 1) {
+          transitionObject();
+        }
+      } finally {
+        ObjectTransitionSafepoint.INSTANCE.unregister();
       }
-
-      ObjectTransitionSafepoint.INSTANCE.unregister();
       return null;
-    }, 60);
+    }, 240);
   }
 
   @Test
   public void testSingleSafepointStorm() throws InterruptedException {
     ObjectTransitionSafepoint.reset();
     ParallelHelper.executeNTimesInParallel((final int id) -> {
+      try {
+        ObjectTransitionSafepoint.INSTANCE.register();
 
-      ObjectTransitionSafepoint.INSTANCE.register();
-
-      for (int i = 0; i < 100_000; i += 1) {
-        if (id == 0) {
-          ObjectTransitionSafepoint.INSTANCE.transitionObject(
-              new SMutableObject(instanceClass, factory, layout));
-        } else {
-          ObjectTransitionSafepoint.INSTANCE.checkAndPerformSafepoint();
+        for (int i = 0; i < 100_000; i += 1) {
+          if (id == 0) {
+            ObjectTransitionSafepoint.INSTANCE.transitionObject(
+                new SMutableObject(instanceClass, factory, layout));
+          } else {
+            ObjectTransitionSafepoint.INSTANCE.checkAndPerformSafepoint();
+          }
         }
+      } finally {
+        ObjectTransitionSafepoint.INSTANCE.unregister();
       }
-
-      ObjectTransitionSafepoint.INSTANCE.unregister();
       return null;
-    }, 60);
+    }, 120);
   }
 
   @Test
   public void testSafepointRegisterStorm() throws InterruptedException {
     ObjectTransitionSafepoint.reset();
     ParallelHelper.executeNTimesInParallel((final int id) -> {
       for (int i = 0; i < 100_000; i += 1) {
-        ObjectTransitionSafepoint.INSTANCE.register();
-
-        ObjectTransitionSafepoint.INSTANCE.transitionObject(
-            new SMutableObject(instanceClass, factory, layout));
+        try {
+          ObjectTransitionSafepoint.INSTANCE.register();
 
-        ObjectTransitionSafepoint.INSTANCE.unregister();
+          ObjectTransitionSafepoint.INSTANCE.transitionObject(
+              new SMutableObject(instanceClass, factory, layout));
+        } finally {
+          ObjectTransitionSafepoint.INSTANCE.unregister();
+        }
       }
       return null;
-    }, 60);
+    }, 120);
   }
 
   @Test
   public void testSingleSafepointRegisterStorm() throws InterruptedException {
     ObjectTransitionSafepoint.reset();
     ParallelHelper.executeNTimesInParallel((final int id) -> {
       for (int i = 0; i < 100_000; i += 1) {
-        ObjectTransitionSafepoint.INSTANCE.register();
-
-        if (id == 0) {
-          ObjectTransitionSafepoint.INSTANCE.transitionObject(
-              new SMutableObject(instanceClass, factory, layout));
-        } else {
-          ObjectTransitionSafepoint.INSTANCE.checkAndPerformSafepoint();
+        try {
+          ObjectTransitionSafepoint.INSTANCE.register();
+
+          if (id == 0) {
+            ObjectTransitionSafepoint.INSTANCE.transitionObject(
+                new SMutableObject(instanceClass, factory, layout));
+          } else {
+            ObjectTransitionSafepoint.INSTANCE.checkAndPerformSafepoint();
+          }
+        } finally {
+          ObjectTransitionSafepoint.INSTANCE.unregister();
         }
-
-        ObjectTransitionSafepoint.INSTANCE.unregister();
       }
       return null;
     }, 60);
@@ -159,8 +174,11 @@ public void testRegisterStorm() throws InterruptedException {
     ObjectTransitionSafepoint.reset();
     ParallelHelper.executeNTimesInParallel((final int id) -> {
       for (int i = 0; i < 100_000; i += 1) {
-        ObjectTransitionSafepoint.INSTANCE.register();
-        ObjectTransitionSafepoint.INSTANCE.unregister();
+        try {
+          ObjectTransitionSafepoint.INSTANCE.register();
+        } finally {
+          ObjectTransitionSafepoint.INSTANCE.unregister();
+        }
       }
       return null;
     }, 60);

diff --git a/tests/java/som/tests/ParallelHelper.java b/tests/java/som/tests/ParallelHelper.java
@@ -24,10 +24,9 @@ public static void executeNTimesInParallel(final IntFunction<Void> task,
     int numThreads = getNumberOfThreads();
 
     ExecutorService threadPool = Executors.newFixedThreadPool(numThreads);
+    List<Throwable> exceptions = Collections.synchronizedList(new ArrayList<Throwable>());
 
     try {
-      List<Throwable> exceptions = Collections.synchronizedList(new ArrayList<Throwable>());
-
       CountDownLatch threadsInitialized = new CountDownLatch(numThreads);
       CountDownLatch threadsDone = new CountDownLatch(numThreads);
 
@@ -41,8 +40,9 @@ public static void executeNTimesInParallel(final IntFunction<Void> task,
             task.apply(id);
           } catch (Throwable t) {
             exceptions.add(t);
+          } finally {
+            threadsDone.countDown();
           }
-          threadsDone.countDown();
         });
       }
       boolean allArrivedWithinTime = threadsDone.await(timeoutInSeconds, TimeUnit.SECONDS);
@@ -53,7 +53,7 @@ public static void executeNTimesInParallel(final IntFunction<Void> task,
       }
 
       assertTrue("Failed parallel test with: " + exceptions, exceptions.isEmpty());
-      assertTrue(allArrivedWithinTime);
+      assertTrue("Some threads timed out on threadsDone", allArrivedWithinTime);
     } finally {
       threadPool.shutdownNow();
     }