From 4b5a0f6557d7152ec770bc13ad3c494449de0529 Mon Sep 17 00:00:00 2001 From: Sven Gothel Date: Sun, 22 Jul 2012 04:16:55 +0200 Subject: Fix Bug 606 - New AWT threading implementation breaks .. ; Fix GLAutoDrawable multi-threading w/ proper pattern (hope so) Considering code changes and remarks: 3ed491213f8f7f05d7b9866b50d764370d8ff5f6 1a91ec5c8b6fd9d9db7bc115569c369fe7b38e9b 3334a924309a9361a448d69bc707d4cce416b430 4f27bcecf7484dc041551f52a5c49e2884cb3867 It seems necessary to have - recursive locking employed for all semantic actions which changes drawable & context (and the Window resource) - to avoid deadlock, we have to ensure the locked code segment will not spawn off to another thread, or a thread holds the lock, spawns of an action requiring the lock. .. sure - other read-only methods (flags, ..) shall at least utilize a safe local copy of a volatile field if further use to produce the result is necessary. - flags like sendReshape require to be volatile to guarantee it's being processed Patch impacts: AWT/SWT GLCanvas, GLAutoDrawableBase [and it's specializations] and hopefully closes any loopholes of missing a cache hit, etc. If you review this and find optimizations, i.e. removing a lock due to semantics etc, don't hold back and discuss it, please. --- src/newt/classes/jogamp/newt/WindowImpl.java | 99 ++++++++++++++-------------- 1 file changed, 51 insertions(+), 48 deletions(-) (limited to 'src/newt/classes/jogamp') diff --git a/src/newt/classes/jogamp/newt/WindowImpl.java b/src/newt/classes/jogamp/newt/WindowImpl.java index 002144b2f..b12e42680 100644 --- a/src/newt/classes/jogamp/newt/WindowImpl.java +++ b/src/newt/classes/jogamp/newt/WindowImpl.java @@ -564,9 +564,11 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer @Override public final int lockSurface() throws NativeWindowException, RuntimeException { - windowLock.lock(); - surfaceLock.lock(); - int res = surfaceLock.getHoldCount() == 1 ? LOCK_SURFACE_NOT_READY : LOCK_SUCCESS; // new lock ? + final RecursiveLock _wlock = windowLock; + final RecursiveLock _slock = surfaceLock; + _wlock.lock(); + _slock.lock(); + int res = _slock.getHoldCount() == 1 ? LOCK_SURFACE_NOT_READY : LOCK_SUCCESS; // new lock ? if ( LOCK_SURFACE_NOT_READY == res ) { try { @@ -583,8 +585,8 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer } } finally { if (LOCK_SURFACE_NOT_READY >= res) { - surfaceLock.unlock(); - windowLock.unlock(); + _slock.unlock(); + _wlock.unlock(); } } } @@ -593,10 +595,12 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer @Override public final void unlockSurface() { - surfaceLock.validateLocked(); - windowLock.validateLocked(); + final RecursiveLock _slock = surfaceLock; + final RecursiveLock _wlock = windowLock; + _slock.validateLocked(); + _wlock.validateLocked(); - if (surfaceLock.getHoldCount() == 1) { + if (_slock.getHoldCount() == 1) { final AbstractGraphicsDevice adevice = getGraphicsConfiguration().getScreen().getDevice(); try { unlockSurfaceImpl(); @@ -604,8 +608,8 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer adevice.unlock(); } } - surfaceLock.unlock(); - windowLock.unlock(); + _slock.unlock(); + _wlock.unlock(); } @Override @@ -618,21 +622,10 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer return surfaceLock.getOwner(); } - public final void lockWindow() { - windowLock.lock(); - } - public final void unlockWindow() { - windowLock.unlock(); + public final RecursiveLock getLock() { + return windowLock; } - public final boolean isWindowLockedByOtherThread() { - return windowLock.isLockedByOtherThread(); - } - - public final Thread getWindowLockOwner() { - return windowLock.getOwner(); - } - public long getSurfaceHandle() { return windowHandle; // default: return window handle } @@ -670,11 +663,12 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer public Point getLocationOnScreen(Point storage) { if(isNativeValid()) { Point d; - windowLock.lock(); + final RecursiveLock _lock = windowLock; + _lock.lock(); try { d = getLocationOnScreenImpl(0, 0); } finally { - windowLock.unlock(); + _lock.unlock(); } if(null!=d) { if(null!=storage) { @@ -717,7 +711,8 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer boolean nativeWindowCreated = false; boolean madeVisible = false; - windowLock.lock(); + final RecursiveLock _lock = windowLock; + _lock.lock(); try { if(null!=lifecycleHook) { lifecycleHook.resetCounter(); @@ -739,7 +734,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer madeVisible = nativeWindowCreated; } // always flag visible, allowing a retry .. - WindowImpl.this.visible = true; + WindowImpl.this.visible = true; } else if(WindowImpl.this.visible != visible) { if(isNativeValid()) { setVisibleImpl(visible, getX(), getY(), getWidth(), getHeight()); @@ -766,7 +761,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer System.err.println("Window setVisible: END ("+getThreadName()+") "+getX()+"/"+getY()+" "+getWidth()+"x"+getHeight()+", fs "+fullscreen+", windowHandle "+toHexString(windowHandle)+", visible: "+WindowImpl.this.visible+", nativeWindowCreated: "+nativeWindowCreated+", madeVisible: "+madeVisible); } } finally { - windowLock.unlock(); + _lock.unlock(); } if( nativeWindowCreated || madeVisible ) { sendWindowEvent(WindowEvent.EVENT_WINDOW_RESIZED); // trigger a resize/relayout and repaint to listener @@ -801,7 +796,8 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer public final void run() { boolean recreate = false; - windowLock.lock(); + final RecursiveLock _lock = windowLock; + _lock.lock(); try { if ( !isFullscreen() && ( getWidth() != width || getHeight() != height ) ) { recreate = isNativeValid() && !getGraphicsConfiguration().getChosenCapabilities().isOnscreen(); @@ -842,7 +838,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer if(recreate) { screen.removeReference(); // bring back ref-count } - windowLock.unlock(); + _lock.unlock(); } } } @@ -863,7 +859,8 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer if(null!=lifecycleHook) { lifecycleHook.destroyActionPreLock(); } - windowLock.lock(); + final RecursiveLock _lock = windowLock; + _lock.lock(); try { if(DEBUG_IMPLEMENTATION) { System.err.println("Window DestroyAction() "+getThreadName()); @@ -917,7 +914,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer hasFocus = false; parentWindowHandle = 0; - windowLock.unlock(); + _lock.unlock(); } if(animatorPaused) { lifecycleHook.resumeRenderingAction(); @@ -1002,8 +999,9 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer int width = getWidth(); int height = getHeight(); boolean wasVisible; - - windowLock.lock(); + + final RecursiveLock _lock = windowLock; + _lock.lock(); try { if(isNativeValid()) { // force recreation if offscreen, since it may become onscreen @@ -1220,7 +1218,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer System.err.println("Window.reparentWindow: END-1 ("+getThreadName()+") windowHandle "+toHexString(windowHandle)+", visible: "+visible+", parentWindowHandle "+toHexString(parentWindowHandle)+", parentWindow "+ Display.hashCodeNullSafe(parentWindow)+" "+x+"/"+y+" "+width+"x"+height); } } finally { - windowLock.unlock(); + _lock.unlock(); } if(wasVisible) { switch (operation) { @@ -1245,7 +1243,8 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer private class ReparentActionRecreate implements Runnable { public final void run() { - windowLock.lock(); + final RecursiveLock _lock = windowLock; + _lock.lock(); try { visible = true; if(DEBUG_IMPLEMENTATION) { @@ -1253,7 +1252,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer } setVisible(true); // native creation } finally { - windowLock.unlock(); + _lock.unlock(); } } } @@ -1291,7 +1290,8 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer } public final void run() { - windowLock.lock(); + final RecursiveLock _lock = windowLock; + _lock.lock(); try { if(WindowImpl.this.undecorated != undecorated) { // set current state @@ -1311,7 +1311,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer } } } finally { - windowLock.unlock(); + _lock.unlock(); } sendWindowEvent(WindowEvent.EVENT_WINDOW_RESIZED); // trigger a resize/relayout and repaint to listener } @@ -1333,7 +1333,8 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer } public final void run() { - windowLock.lock(); + final RecursiveLock _lock = windowLock; + _lock.lock(); try { if(WindowImpl.this.alwaysOnTop != alwaysOnTop) { // set current state @@ -1353,7 +1354,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer } } } finally { - windowLock.unlock(); + _lock.unlock(); } sendWindowEvent(WindowEvent.EVENT_WINDOW_RESIZED); // trigger a resize/relayout and repaint to listener } @@ -1545,7 +1546,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer "\n, ParentWindow "+parentWindow+ "\n, ParentWindowHandle "+toHexString(parentWindowHandle)+" ("+(0!=getParentWindowHandle())+")"+ "\n, WindowHandle "+toHexString(getWindowHandle())+ - "\n, SurfaceHandle "+toHexString(getSurfaceHandle())+ " (lockedExt window "+isWindowLockedByOtherThread()+", surface "+isSurfaceLockedByOtherThread()+")"+ + "\n, SurfaceHandle "+toHexString(getSurfaceHandle())+ " (lockedExt window "+windowLock.isLockedByOtherThread()+", surface "+isSurfaceLockedByOtherThread()+")"+ "\n, Pos "+getX()+"/"+getY()+" (auto "+autoPosition()+"), size "+getWidth()+"x"+getHeight()+ "\n, Visible "+isVisible()+", focus "+hasFocus()+ "\n, Undecorated "+undecorated+" ("+isUndecorated()+")"+ @@ -1675,7 +1676,8 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer } public final void run() { - windowLock.lock(); + final RecursiveLock _lock = windowLock; + _lock.lock(); try { if(DEBUG_IMPLEMENTATION) { System.err.println("Window setPosition: "+getX()+"/"+getY()+" -> "+x+"/"+y+", fs "+fullscreen+", windowHandle "+toHexString(windowHandle)); @@ -1689,7 +1691,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer } } } finally { - windowLock.unlock(); + _lock.unlock(); } } } @@ -1718,7 +1720,8 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer public boolean fsOn() { return fullscreen; } public final void run() { - windowLock.lock(); + final RecursiveLock _lock = windowLock; + _lock.lock(); try { // set current state WindowImpl.this.fullscreen = fullscreen; @@ -1795,7 +1798,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer } } } finally { - windowLock.unlock(); + _lock.unlock(); } sendWindowEvent(WindowEvent.EVENT_WINDOW_RESIZED); // trigger a resize/relayout and repaint to listener } @@ -1917,7 +1920,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer // special repaint treatment case WindowEvent.EVENT_WINDOW_REPAINT: // queue repaint event in case window is locked, ie in operation - if( null != getWindowLockOwner() ) { + if( null != windowLock.getOwner() ) { // make sure only one repaint event is queued if(!repaintQueued) { repaintQueued=true; @@ -1936,7 +1939,7 @@ public abstract class WindowImpl implements Window, NEWTEventConsumer // common treatment case WindowEvent.EVENT_WINDOW_RESIZED: // queue event in case window is locked, ie in operation - if( null != getWindowLockOwner() ) { + if( null != windowLock.getOwner() ) { final boolean discardTO = QUEUED_EVENT_TO <= System.currentTimeMillis()-e.getWhen(); if(DEBUG_IMPLEMENTATION) { System.err.println("Window.consumeEvent: "+Thread.currentThread().getName()+" - queued "+e+", discard-to "+discardTO); -- cgit v1.2.3