Simplify Animator* Synchronization: Remove barrier 'stateSync' and favor simple 'synchronized' on Animator for field-get, which is already used in most methods

Utilizing a 2nd synchronization object 'stateSync' besides the main sync object, Animator itself,
is hard to maintain. It's performance advantages for querying states ae questionable and may even
introduce bugs.

Use synchronization on Animator instance for all field read/write access.

Fix unsynchronized write access of 'animThread' in Animator.MainLoop.run().

3 files changed, 81 insertions, 162 deletions

diff --git a/src/jogl/classes/com/jogamp/opengl/util/Animator.java b/src/jogl/classes/com/jogamp/opengl/util/Animator.java
index 27b9427eb..799069292 100644
--- a/src/jogl/classes/com/jogamp/opengl/util/Animator.java
+++ b/src/jogl/classes/com/jogamp/opengl/util/Animator.java
@@ -110,7 +110,7 @@ public class Animator extends AnimatorBase {
     }
 
     @Override
-    protected String getBaseName(String prefix) {
+    protected final String getBaseName(String prefix) {
         return prefix + "Animator" ;
     }
 
@@ -120,40 +120,28 @@ public class Animator extends AnimatorBase {
      * animation loop which prevents the CPU from getting swamped.
      * This method may not have an effect on subclasses.
      */
-    public final void setRunAsFastAsPossible(boolean runFast) {
-        stateSync.lock();
-        try {
-            runAsFastAsPossible = runFast;
-        } finally {
-            stateSync.unlock();
-        }
-    }
-
-    private final void setIsAnimatingSynced(boolean v) {
-        stateSync.lock();
-        try {
-            isAnimating = v;
-        } finally {
-            stateSync.unlock();
-        }
+    public final synchronized void setRunAsFastAsPossible(boolean runFast) {
+        runAsFastAsPossible = runFast;
     }
 
     class MainLoop implements Runnable {
         @Override
         public String toString() {
-            return "[started "+isStartedImpl()+", animating "+isAnimatingImpl()+", paused "+isPausedImpl()+", drawable "+drawables.size()+", drawablesEmpty "+drawablesEmpty+"]";
+            return "[started "+isStarted()+", animating "+isAnimating()+", paused "+isPaused()+", drawable "+drawables.size()+", drawablesEmpty "+drawablesEmpty+"]";
         }
 
         @Override
         public void run() {
             try {
-                if(DEBUG) {
-                    System.err.println("Animator start on " + getThreadName() + ": " + toString());
+                synchronized (Animator.this) {
+                    if(DEBUG) {
+                        System.err.println("Animator start on " + getThreadName() + ": " + toString());
+                    }
+                    fpsCounter.resetFPSCounter();
+                    animThread = Thread.currentThread();
+                    isAnimating = false;
+                    // 'waitForStartedCondition' wake-up is handled below!
                 }
-                fpsCounter.resetFPSCounter();
-                animThread = Thread.currentThread();
-                setIsAnimatingSynced(false); // barrier
-                // 'waitForStartedCondition' wake-up is handled below!
 
                 while (!stopIssued) {
                     synchronized (Animator.this) {
@@ -172,7 +160,7 @@ public class Animator extends AnimatorBase {
                                 setDrawablesExclCtxState(false);
                                 display(); // propagate exclusive change!
                             }
-                            setIsAnimatingSynced(false); // barrier
+                            isAnimating = false;
                             Animator.this.notifyAll();
                             try {
                                 Animator.this.wait();
@@ -191,7 +179,7 @@ public class Animator extends AnimatorBase {
                             // - and -
                             // Resume from pause or drawablesEmpty,
                             // implies !pauseIssued and !drawablesEmpty
-                            setIsAnimatingSynced(true); // barrier
+                            isAnimating = true;
                             setDrawablesExclCtxState(exclusiveContext);
                             Animator.this.notifyAll();
                         }
@@ -221,37 +209,21 @@ public class Animator extends AnimatorBase {
                     stopIssued = false;
                     pauseIssued = false;
                     animThread = null;
-                    setIsAnimatingSynced(false); // barrier
+                    isAnimating = false;
                     Animator.this.notifyAll();
                 }
             }
         }
     }
 
-    private final boolean isAnimatingImpl() {
-        return animThread != null && isAnimating ;
-    }
     @Override
-    public final boolean isAnimating() {
-        stateSync.lock();
-        try {
-            return animThread != null && isAnimating ;
-        } finally {
-            stateSync.unlock();
-        }
+    public final synchronized boolean isAnimating() {
+        return animThread != null && isAnimating ;
     }
 
-    private final boolean isPausedImpl() {
-        return animThread != null && pauseIssued ;
-    }
     @Override
-    public final boolean isPaused() {
-        stateSync.lock();
-        try {
-            return animThread != null && pauseIssued ;
-        } finally {
-            stateSync.unlock();
-        }
+    public final synchronized boolean isPaused() {
+        return animThread != null && pauseIssued ;
     }
 
     /**
@@ -260,16 +232,16 @@ public class Animator extends AnimatorBase {
      * @param tg the {@link ThreadGroup}
      * @throws GLException if the animator has already been started
      */
-    public synchronized void setThreadGroup(ThreadGroup tg) throws GLException {
-        if ( isStartedImpl() ) {
+    public final synchronized void setThreadGroup(ThreadGroup tg) throws GLException {
+        if ( isStarted() ) {
             throw new GLException("Animator already started.");
         }
         threadGroup = tg;
     }
 
     @Override
-    public synchronized boolean start() {
-        if ( isStartedImpl() ) {
+    public final synchronized boolean start() {
+        if ( isStarted() ) {
             return false;
         }
         if (runnable == null) {
@@ -294,12 +266,12 @@ public class Animator extends AnimatorBase {
     private final Condition waitForStartedCondition = new Condition() {
         @Override
         public boolean eval() {
-            return !isStartedImpl() || (!drawablesEmpty && !isAnimating) ;
+            return !isStarted() || (!drawablesEmpty && !isAnimating) ;
         } };
 
     @Override
-    public synchronized boolean stop() {
-        if ( !isStartedImpl() ) {
+    public final synchronized boolean stop() {
+        if ( !isStarted() ) {
             return false;
         }
         stopIssued = true;
@@ -308,12 +280,12 @@ public class Animator extends AnimatorBase {
     private final Condition waitForStoppedCondition = new Condition() {
         @Override
         public boolean eval() {
-            return isStartedImpl();
+            return isStarted();
         } };
 
     @Override
-    public synchronized boolean pause() {
-        if ( !isStartedImpl() || pauseIssued ) {
+    public final synchronized boolean pause() {
+        if ( !isStarted() || pauseIssued ) {
             return false;
         }
         pauseIssued = true;
@@ -323,12 +295,12 @@ public class Animator extends AnimatorBase {
         @Override
         public boolean eval() {
             // end waiting if stopped as well
-            return isStartedImpl() && isAnimating;
+            return isStarted() && isAnimating;
         } };
 
     @Override
-    public synchronized boolean resume() {
-        if ( !isStartedImpl() || !pauseIssued ) {
+    public final synchronized boolean resume() {
+        if ( !isStarted() || !pauseIssued ) {
             return false;
         }
         pauseIssued = false;
@@ -338,6 +310,6 @@ public class Animator extends AnimatorBase {
         @Override
         public boolean eval() {
             // end waiting if stopped as well
-            return isStartedImpl() && ( !drawablesEmpty && !isAnimating || drawablesEmpty && !pauseIssued ) ;
+            return isStarted() && ( !drawablesEmpty && !isAnimating || drawablesEmpty && !pauseIssued ) ;
         } };
 }
diff --git a/src/jogl/classes/com/jogamp/opengl/util/AnimatorBase.java b/src/jogl/classes/com/jogamp/opengl/util/AnimatorBase.java
index ee2754bdf..39643744a 100644
--- a/src/jogl/classes/com/jogamp/opengl/util/AnimatorBase.java
+++ b/src/jogl/classes/com/jogamp/opengl/util/AnimatorBase.java
@@ -28,8 +28,6 @@
 
 package com.jogamp.opengl.util;
 
-import com.jogamp.common.util.locks.LockFactory;
-import com.jogamp.common.util.locks.RecursiveLock;
 import jogamp.opengl.Debug;
 import jogamp.opengl.FPSCounterImpl;
 
@@ -86,7 +84,6 @@ public abstract class AnimatorBase implements GLAnimatorControl {
     protected boolean exclusiveContext;
     protected Thread userExclusiveContextThread;
     protected FPSCounterImpl fpsCounter = new FPSCounterImpl();
-    protected RecursiveLock stateSync = LockFactory.createRecursiveLock();
 
     private final static Class<?> awtAnimatorImplClazz;
     static {
@@ -129,7 +126,7 @@ public abstract class AnimatorBase implements GLAnimatorControl {
      *
      * @throws GLException if Animator is {@link #isStarted()}
      */
-    protected synchronized void initImpl(boolean force) {
+    protected final synchronized void initImpl(boolean force) {
         if( force || null == impl ) {
             if( useAWTAnimatorImpl( modeBits ) ) {
                 try {
@@ -157,7 +154,7 @@ public abstract class AnimatorBase implements GLAnimatorControl {
      * @throws GLException if Animator is {@link #isStarted()} and {@link #MODE_EXPECT_AWT_RENDERING_THREAD} about to change
      * @see AnimatorBase#MODE_EXPECT_AWT_RENDERING_THREAD
      */
-    public synchronized void setModeBits(boolean enable, int bitValues) throws GLException {
+    public final synchronized void setModeBits(boolean enable, int bitValues) throws GLException {
         final int _oldModeBits = modeBits;
         if(enable) {
             modeBits |=  bitValues;
@@ -175,7 +172,7 @@ public abstract class AnimatorBase implements GLAnimatorControl {
 
 
     @Override
-    public synchronized void add(final GLAutoDrawable drawable) {
+    public final synchronized void add(final GLAutoDrawable drawable) {
         if(DEBUG) {
             System.err.println("Animator add: 0x"+Integer.toHexString(drawable.hashCode())+" - "+toString()+" - "+getThreadName());
         }
@@ -207,7 +204,7 @@ public abstract class AnimatorBase implements GLAnimatorControl {
     }
 
     @Override
-    public synchronized void remove(final GLAutoDrawable drawable) {
+    public final synchronized void remove(final GLAutoDrawable drawable) {
         if(DEBUG) {
             System.err.println("Animator remove: 0x"+Integer.toHexString(drawable.hashCode())+" - "+toString()+" - "+getThreadName());
         }
@@ -270,17 +267,11 @@ public abstract class AnimatorBase implements GLAnimatorControl {
      * @see #isExclusiveContextEnabled()
      */
     // @Override
-    public final Thread setExclusiveContext(Thread t) {
-        final Thread old;
+    public final synchronized Thread setExclusiveContext(Thread t) {
         final boolean enable = null != t;
-        stateSync.lock();
-        try {
-            old = userExclusiveContextThread;
-            if( enable && t != animThread ) { // disable: will be cleared at end after propagation && filter out own animThread usae
-                userExclusiveContextThread=t;
-            }
-        } finally {
-            stateSync.unlock();
+        final Thread old = userExclusiveContextThread;
+        if( enable && t != animThread ) { // disable: will be cleared at end after propagation && filter out own animThread usae
+            userExclusiveContextThread=t;
         }
         setExclusiveContext(enable);
         return old;
@@ -340,11 +331,8 @@ public abstract class AnimatorBase implements GLAnimatorControl {
                         pause();
                     }
                 }
-                stateSync.lock();
-                try {
+                synchronized(AnimatorBase.this) {
                     userExclusiveContextThread=null;
-                } finally {
-                    stateSync.unlock();
                 }
             }
         }
@@ -361,13 +349,8 @@ public abstract class AnimatorBase implements GLAnimatorControl {
      * @see #setExclusiveContext(Thread)
      */
     // @Override
-    public final boolean isExclusiveContextEnabled() {
-        stateSync.lock();
-        try {
-            return exclusiveContext;
-        } finally {
-            stateSync.unlock();
-        }
+    public final synchronized boolean isExclusiveContextEnabled() {
+        return exclusiveContext;
     }
 
     /**
@@ -384,13 +367,8 @@ public abstract class AnimatorBase implements GLAnimatorControl {
      * @see #setExclusiveContext(Thread)
      */
     // @Override
-    public final Thread getExclusiveContextThread() {
-        stateSync.lock();
-        try {
-            return ( isStartedImpl() && exclusiveContext ) ? ( null != userExclusiveContextThread ? userExclusiveContextThread : animThread ) : null ;
-        } finally {
-            stateSync.unlock();
-        }
+    public final synchronized Thread getExclusiveContextThread() {
+        return ( isStarted() && exclusiveContext ) ? ( null != userExclusiveContextThread ? userExclusiveContextThread : animThread ) : null ;
     }
 
     /**
@@ -425,13 +403,8 @@ public abstract class AnimatorBase implements GLAnimatorControl {
     }
 
     @Override
-    public final Thread getThread() {
-        stateSync.lock();
-        try {
-            return animThread;
-        } finally {
-            stateSync.unlock();
-        }
+    public final synchronized Thread getThread() {
+        return animThread;
     }
 
     /** Called every frame to cause redrawing of all of the
@@ -439,7 +412,7 @@ public abstract class AnimatorBase implements GLAnimatorControl {
         this to get the most optimized painting behavior for the set of
         components this Animator manages, in particular when multiple
         lightweight widgets are continually being redrawn. */
-    protected void display() {
+    protected final void display() {
         impl.display(drawables, ignoreExceptions, printExceptions);
         fpsCounter.tickFPS();
     }
@@ -497,7 +470,7 @@ public abstract class AnimatorBase implements GLAnimatorControl {
     /** Sets a flag causing this Animator to ignore exceptions produced
     while redrawing the drawables. By default this flag is set to
     false, causing any exception thrown to halt the Animator. */
-    public void setIgnoreExceptions(boolean ignoreExceptions) {
+    public final void setIgnoreExceptions(boolean ignoreExceptions) {
         this.ignoreExceptions = ignoreExceptions;
     }
 
@@ -505,7 +478,7 @@ public abstract class AnimatorBase implements GLAnimatorControl {
     this Animator (see {@link #setIgnoreExceptions}), to print the
     exceptions' stack traces for diagnostic information. Defaults to
     false. */
-    public void setPrintExceptions(boolean printExceptions) {
+    public final void setPrintExceptions(boolean printExceptions) {
         this.printExceptions = printExceptions;
     }
 
@@ -522,7 +495,7 @@ public abstract class AnimatorBase implements GLAnimatorControl {
      *                   if &gt; <code>0</code>, method will wait for the given <code>pollPeriod</code> in milliseconds.
      * @return <code>true</code> if {@link Condition#eval() waitCondition.eval()} returned <code>false</code>, otherwise <code>false</code>.
      */
-    protected synchronized boolean finishLifecycleAction(Condition waitCondition, long pollPeriod) {
+    protected final synchronized boolean finishLifecycleAction(Condition waitCondition, long pollPeriod) {
         /**
          * It's hard to tell whether the thread which changes the lifecycle has
          * dependencies on the Animator's internal thread. Currently we
@@ -570,16 +543,11 @@ public abstract class AnimatorBase implements GLAnimatorControl {
             if( blocking && remaining<=0 && nok ) {
                 System.err.println("finishLifecycleAction(" + waitCondition.getClass().getName() + "): ++++++ timeout reached ++++++ " + getThreadName());
             }
-            stateSync.lock(); // avoid too many lock/unlock ops
-            try {
-                System.err.println("finishLifecycleAction(" + waitCondition.getClass().getName() + "): OK "+(!nok)+
-                        "- pollPeriod "+pollPeriod+", blocking "+blocking+
-                        ", waited " + (blocking ? ( TO_WAIT_FOR_FINISH_LIFECYCLE_ACTION - remaining ) : 0 ) + "/" + TO_WAIT_FOR_FINISH_LIFECYCLE_ACTION +
-                        " - " + getThreadName());
-                System.err.println(" - "+toString());
-            } finally {
-                stateSync.unlock();
-            }
+            System.err.println("finishLifecycleAction(" + waitCondition.getClass().getName() + "): OK "+(!nok)+
+                    "- pollPeriod "+pollPeriod+", blocking "+blocking+
+                    ", waited " + (blocking ? ( TO_WAIT_FOR_FINISH_LIFECYCLE_ACTION - remaining ) : 0 ) + "/" + TO_WAIT_FOR_FINISH_LIFECYCLE_ACTION +
+                    " - " + getThreadName());
+            System.err.println(" - "+toString());
             if(nok) {
                 Thread.dumpStack();
             }
@@ -587,17 +555,9 @@ public abstract class AnimatorBase implements GLAnimatorControl {
         return !nok;
     }
 
-    protected final boolean isStartedImpl() {
-        return animThread != null ;
-    }
     @Override
-    public boolean isStarted() {
-        stateSync.lock();
-        try {
-            return animThread != null ;
-        } finally {
-            stateSync.unlock();
-        }
+    public synchronized boolean isStarted() {
+        return animThread != null ;
     }
 
     protected static String getThreadName() { return Thread.currentThread().getName(); }
diff --git a/src/jogl/classes/com/jogamp/opengl/util/FPSAnimator.java b/src/jogl/classes/com/jogamp/opengl/util/FPSAnimator.java
index 351c47e7e..65fed17f2 100644
--- a/src/jogl/classes/com/jogamp/opengl/util/FPSAnimator.java
+++ b/src/jogl/classes/com/jogamp/opengl/util/FPSAnimator.java
@@ -59,7 +59,7 @@ public class FPSAnimator extends AnimatorBase {
     private Timer timer = null;
     private MainTask task = null;
     private int fps;
-    private boolean scheduleAtFixedRate;
+    private final boolean scheduleAtFixedRate;
     private boolean isAnimating;         // MainTask feedback
     private volatile boolean shouldRun;  // MainTask trigger
     private volatile boolean shouldStop; // MainTask trigger
@@ -107,7 +107,7 @@ public class FPSAnimator extends AnimatorBase {
      * @throws GLException if the animator has already been started
      */
     public final synchronized void setFPS(int fps) throws GLException {
-        if ( isStartedImpl() ) {
+        if ( isStarted() ) {
             throw new GLException("Animator already started.");
         }
         this.fps = fps;
@@ -131,7 +131,7 @@ public class FPSAnimator extends AnimatorBase {
             alreadyStopped = false;
             alreadyPaused = false;
 
-            final long period = 0 < fps ? (long) (1000.0f / (float) fps) : 1; // 0 -> 1: IllegalArgumentException: Non-positive period
+            final long period = 0 < fps ? (long) (1000.0f / fps) : 1; // 0 -> 1: IllegalArgumentException: Non-positive period
             if (scheduleAtFixedRate) {
                 timer.scheduleAtFixedRate(this, 0, period);
             } else {
@@ -142,8 +142,8 @@ public class FPSAnimator extends AnimatorBase {
         public boolean isActive() { return !alreadyStopped && !alreadyPaused; }
 
         @Override
-        public String toString() {
-            return "Task[thread "+animThread+", stopped "+alreadyStopped+", paused "+alreadyPaused+" shouldRun "+shouldRun+", shouldStop "+shouldStop+" -- started "+isStartedImpl()+", animating "+isAnimatingImpl()+", paused "+isPausedImpl()+", drawable "+drawables.size()+", drawablesEmpty "+drawablesEmpty+"]";
+        public final String toString() {
+            return "Task[thread "+animThread+", stopped "+alreadyStopped+", paused "+alreadyPaused+" shouldRun "+shouldRun+", shouldStop "+shouldStop+" -- started "+isStarted()+", animating "+isAnimatingImpl()+", paused "+isPaused()+", drawable "+drawables.size()+", drawablesEmpty "+drawablesEmpty+"]";
         }
 
         @Override
@@ -212,33 +212,20 @@ public class FPSAnimator extends AnimatorBase {
         return animThread != null && isAnimating ;
     }
     @Override
-    public final boolean isAnimating() {
-        stateSync.lock();
-        try {
-            return animThread != null && isAnimating ;
-        } finally {
-            stateSync.unlock();
-        }
+    public final synchronized boolean isAnimating() {
+        return animThread != null && isAnimating ;
     }
 
-    private final boolean isPausedImpl() {
-        return animThread != null && ( !shouldRun && !shouldStop ) ;
-    }
     @Override
-    public final boolean isPaused() {
-        stateSync.lock();
-        try {
-            return animThread != null && ( !shouldRun && !shouldStop ) ;
-        } finally {
-            stateSync.unlock();
-        }
+    public final synchronized boolean isPaused() {
+        return animThread != null && ( !shouldRun && !shouldStop ) ;
     }
 
     static int timerNo = 0;
 
     @Override
-    public synchronized boolean start() {
-        if ( null != timer || null != task || isStartedImpl() ) {
+    public final synchronized boolean start() {
+        if ( null != timer || null != task || isStarted() ) {
             return false;
         }
         timer = new Timer( getThreadName()+"-"+baseName+"-Timer"+(timerNo++) );
@@ -262,20 +249,20 @@ public class FPSAnimator extends AnimatorBase {
     private final Condition waitForStartedAddedCondition = new Condition() {
         @Override
         public boolean eval() {
-            return !isStartedImpl() || !isAnimating ;
+            return !isStarted() || !isAnimating ;
         } };
     private final Condition waitForStartedEmptyCondition = new Condition() {
         @Override
         public boolean eval() {
-            return !isStartedImpl() || isAnimating ;
+            return !isStarted() || isAnimating ;
         } };
 
     /** Stops this FPSAnimator. Due to the implementation of the
     FPSAnimator it is not guaranteed that the FPSAnimator will be
     completely stopped by the time this method returns. */
     @Override
-    public synchronized boolean stop() {
-        if ( null == timer || !isStartedImpl() ) {
+    public final synchronized boolean stop() {
+        if ( null == timer || !isStarted() ) {
             return false;
         }
         if(DEBUG) {
@@ -308,12 +295,12 @@ public class FPSAnimator extends AnimatorBase {
     private final Condition waitForStoppedCondition = new Condition() {
         @Override
         public boolean eval() {
-            return isStartedImpl();
+            return isStarted();
         } };
 
     @Override
-    public synchronized boolean pause() {
-        if ( !isStartedImpl() || ( null != task && isPausedImpl() ) ) {
+    public final synchronized boolean pause() {
+        if ( !isStarted() || ( null != task && isPaused() ) ) {
             return false;
         }
         if(DEBUG) {
@@ -341,12 +328,12 @@ public class FPSAnimator extends AnimatorBase {
         @Override
         public boolean eval() {
             // end waiting if stopped as well
-            return isAnimating && isStartedImpl();
+            return isAnimating && isStarted();
         } };
 
     @Override
-    public synchronized boolean resume() {
-        if ( null != task || !isStartedImpl() || !isPausedImpl() ) {
+    public final synchronized boolean resume() {
+        if ( null != task || !isStarted() || !isPaused() ) {
             return false;
         }
         if(DEBUG) {
@@ -369,6 +356,6 @@ public class FPSAnimator extends AnimatorBase {
         @Override
         public boolean eval() {
             // end waiting if stopped as well
-            return !drawablesEmpty && !isAnimating && isStartedImpl();
+            return !drawablesEmpty && !isAnimating && isStarted();
         } };
 }