Bug 519 - GLPBuffer takes awtlock during painting
Summary: GLPBuffer takes awtlock during painting
Status: VERIFIED FIXED
Alias: None
Product: Jogl
Classification: JogAmp
Component: awt (show other bugs)
Version: 2
Hardware: All all
: --- normal
Assignee: Sven Gothel
URL:
Depends on:
Blocks:
 
Reported: 2011-10-03 13:30 CEST by Daniel Balog
Modified: 2011-12-02 07:59 CET (History)
1 user (show)

See Also:
Type: ---
SCM Refs:
0bcf2c35e96cbf7a1e2537448f9603d7d487cf86 e341ad8db546530b3a49c56c32cc26980e296201 c84e235b3284d0e18481748b44594116e25821a9 a94c1a2945a31aa5d93e354da3bc80f59f253ec4 7ae0f2df39692e82d7955dbcd09c35c36382726c 6b8f6e8d7c548cb6bfed14d8a04c9cf252ca7c4d 9ef0a0c185ace5217efc014809e97c5eead842cc 0314be79a7a93931a74fe4322bc78e699d7741e9
Workaround: ---


Attachments
Test that demonstrates deadlock. (2.11 KB, text/x-java)
2011-10-03 13:30 CEST, Daniel Balog
Details
A patch that prevents a PBuffer from taking awtlocks during display (6.72 KB, patch)
2011-10-17 09:27 CEST, Daniel Balog
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Balog 2011-10-03 13:30:43 CEST
Created attachment 276 [details]
Test that demonstrates deadlock.

As described in this thread:

http://forum.jogamp.org/2-0-RC3-PBuffer-uses-awtlock-during-display-td3355580.html

A GLPBuffer takes an AWTLock, even though it is generally used for offscreen rendering. This can cause deadlocks and hangs when offscreen images are being rendered asynchronously, together with AWT/Swing components.

One workaround is to call GLProfile.initSingleton(true) as first line of your application, but this is not always feasible.

One suggestion is to remove AWT locking for offscreen PBuffers altogether.

The relevant stack trace is as follows:

  java.lang.Thread.State: WAITING
          at sun.misc.Unsafe.park(Unsafe.java:-1)
          at java.util.concurrent.locks.LockSupport.park(LockSupport.java:158)
          at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:811)
          at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:842)
          at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1178)
          at java.util.concurrent.locks.ReentrantLock$NonfairSync.lock(ReentrantLock.java:186)
          at java.util.concurrent.locks.ReentrantLock.lock(ReentrantLock.java:262)
          at sun.awt.SunToolkit.awtLock(SunToolkit.java:236)
          at sun.reflect.GeneratedMethodAccessor4.invoke(Unknown Source:-1)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at jogamp.nativewindow.jawt.JAWTUtil.awtLock(JAWTUtil.java:195)
          at jogamp.nativewindow.jawt.JAWTUtil.lockToolkit(JAWTUtil.java:224)
          at jogamp.nativewindow.jawt.x11.X11JAWTToolkitLock.lock(X11JAWTToolkitLock.java:51)
          at javax.media.nativewindow.DefaultGraphicsDevice.lock(DefaultGraphicsDevice.java:128)
          at javax.media.nativewindow.ProxySurface.lockSurface(ProxySurface.java:94)
          at jogamp.opengl.GLDrawableImpl.lockSurface(GLDrawableImpl.java:186)
          at jogamp.opengl.GLContextImpl.makeCurrentLocking(GLContextImpl.java:435)
          at jogamp.opengl.GLContextImpl.makeCurrent(GLContextImpl.java:388)
          at jogamp.opengl.GLDrawableHelper.invokeGL(GLDrawableHelper.java:328)
          at jogamp.opengl.GLPbufferImpl.invokeGL(GLPbufferImpl.java:289)
          at jogamp.opengl.GLPbufferImpl.display(GLPbufferImpl.java:148)


To reproduce the issue, run the code in the attachment.
Comment 1 Daniel Balog 2011-10-17 09:08:37 CEST
So to get this ball rolling, I tried to create a flag in GLContextImpl that basically prevents a surfacelock from being taken when set to false.

This would make the makeCurrentLocking() method look like this:

 // Note: the surface is locked within [makeCurrent .. swap .. release]
  protected final int makeCurrentLocking() throws GLException {
    boolean exceptionOccurred = false;
    int lockRes = 0;
    if ( surfaceLocking ) {
      lockRes = drawable.lockSurface();
    }else{
      lockRes = NativeSurface.LOCK_SUCCESS;
    }
    try {
      if (NativeSurface.LOCK_SURFACE_NOT_READY == lockRes) {
        return CONTEXT_NOT_CURRENT;
      }
      try {
          if (NativeSurface.LOCK_SURFACE_CHANGED == lockRes) {
            drawable.updateHandle();
          }
          if (0 == drawable.getHandle()) {
              throw new GLException("drawable has invalid handle: "+drawable);
          }
          boolean newCreated = false;
          if (!isCreated()) {
            GLProfile.initProfiles(
                    getGLDrawable().getNativeSurface().getGraphicsConfiguration().getNativeGraphicsConfiguration().getScreen().getDevice());
            newCreated = createImpl(); // may throws exception if fails!
            if (DEBUG) {
                if(newCreated) {
                    System.err.println(getThreadName() + ": !!! Create GL context OK: " + toHexString(contextHandle) + " for " + getClass().getName());
                } else {
                    System.err.println(getThreadName() + ": !!! Create GL context FAILED for " + getClass().getName());
                }
            }
            if(!newCreated) {
                return CONTEXT_NOT_CURRENT;
            }
            GLContextShareSet.contextCreated(this);
          }
          makeCurrentImpl(newCreated);
          return newCreated ? CONTEXT_CURRENT_NEW : CONTEXT_CURRENT ;
      } catch (RuntimeException e) {
        exceptionOccurred = true;
        throw e;
      }
    } finally {
      if (exceptionOccurred) {
        if ( surfaceLocking ) {
          drawable.unlockSurface();
        }
      }
    }
  }

The problem is that this method still takes an awtlock regardless, through the following stack trace:

"pool-1-thread-1" prio=10 tid=0x00000000411d4800 nid=0x1844 waiting on condition [0x00007f6511210000]
   java.lang.Thread.State: WAITING (parking)
	at sun.misc.Unsafe.park(Native Method)
	- parking to wait for  <0x00000000bc0b4128> (a java.util.concurrent.locks.ReentrantLock$NonfairSync)
	at java.util.concurrent.locks.LockSupport.park(LockSupport.java:158)
	at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:811)
	at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:842)
	at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1178)
	at java.util.concurrent.locks.ReentrantLock$NonfairSync.lock(ReentrantLock.java:186)
	at java.util.concurrent.locks.ReentrantLock.lock(ReentrantLock.java:262)
	at sun.awt.SunToolkit.awtLock(SunToolkit.java:236)
	at sun.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at jogamp.nativewindow.jawt.JAWTUtil.awtLock(JAWTUtil.java:195)
	at jogamp.nativewindow.jawt.JAWTUtil.lockToolkit(JAWTUtil.java:224)
	at jogamp.nativewindow.jawt.JAWTToolkitLock.lock(JAWTToolkitLock.java:47)
	at jogamp.nativewindow.x11.X11Util.lockDefaultToolkit(X11Util.java:131)
	at jogamp.nativewindow.x11.X11Util.XSync(X11Util.java:502)
	at jogamp.opengl.x11.glx.X11GLXContext.createContextARBImpl(X11GLXContext.java:249)
	at jogamp.opengl.GLContextImpl.createContextARB(GLContextImpl.java:593)
	at jogamp.opengl.x11.glx.X11GLXContext.createImplRaw(X11GLXContext.java:340)
	at jogamp.opengl.x11.glx.X11GLXContext.createImpl(X11GLXContext.java:285)
	at jogamp.opengl.GLContextImpl.makeCurrentLocking(GLContextImpl.java:466)
	at jogamp.opengl.GLContextImpl.makeCurrent(GLContextImpl.java:398)
	at jogamp.opengl.GLDrawableHelper.invokeGL(GLDrawableHelper.java:328)
	at jogamp.opengl.GLPbufferImpl.invokeGL(GLPbufferImpl.java:290)
	at jogamp.opengl.GLPbufferImpl.display(GLPbufferImpl.java:149)
	at com.jogamp.opengl.test.junit.jogl.demos.gl2.awt.TestPBufferDeadLock$1.run(TestPBufferDeadLock.java:75)
	at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
	at java.lang.Thread.run(Thread.java:662)


Basically, the X11Util.XSync method also locks awt, which fails this test.
Comment 2 Daniel Balog 2011-10-17 09:19:55 CEST
Ok, so I can get around the XSync lock by adding a lock parameter to the method that, when set to false, prevents locking. So basically:

    public static int XSync( long display, boolean discard, boolean lock ) {
      if ( lock ) {
        lockDefaultToolkit(display);
      }
      try {
            return X11Lib.XSync(display, discard);
        } finally {
        if ( lock ) {
          unlockDefaultToolkit(display);
        }
      }
    }

And then in X11GLXContext.createContextARBIMpl(), you call:


    try {
        // critical path, a remote display might not support this command,
        // hence we need to catch the X11 Error within this block.
        X11Util.XSync(display, false, isSurfaceLocking() );
        ctx = _glXExt.glXCreateContextAttribsARB(display, config.getFBConfig(), share, direct, attribs);
        X11Util.XSync(display, false, isSurfaceLocking() );
    } catch (RuntimeException re) {
        if(DEBUG) {
          Throwable t = new Throwable("Info: X11GLXContext.createContextARBImpl glXCreateContextAttribsARB failed with "+getGLVersion(major, minor, ctp, "@creation"), re);
          t.printStackTrace();
        }
    }

where isSurfaceLocking() is defined in GLContextImpl as 

  public boolean isSurfaceLocking() {
    return surfaceLocking;
  }

  public void setSurfaceLocking( boolean aSurfaceLocking ) {
    surfaceLocking = aSurfaceLocking;
  }

, and surfaceLocking is a flag that is set to false when creating a PBufferDrawable. So the constructor of GLPBufferImpl would be:

  public GLPbufferImpl(GLDrawableImpl pbufferDrawable,
                       GLContext parentContext) {
    GLCapabilitiesImmutable caps = (GLCapabilitiesImmutable)
         pbufferDrawable.getNativeSurface().getGraphicsConfiguration().getNativeGraphicsConfiguration().getChosenCapabilities();
    if(caps.isOnscreen()) {
        if(caps.isPBuffer()) {
            throw new IllegalArgumentException("Error: Given drawable is Onscreen and Pbuffer: "+pbufferDrawable);
        }
        throw new IllegalArgumentException("Error: Given drawable is Onscreen: "+pbufferDrawable);
    } else {
        if(!caps.isPBuffer()) {
            throw new IllegalArgumentException("Error: Given drawable is not Pbuffer: "+pbufferDrawable);
        }
    }
    this.pbufferDrawable = pbufferDrawable;
    context = (GLContextImpl) pbufferDrawable.createContext(parentContext);
    context.setSynchronized(true);
    context.setSurfaceLocking( false );
  }

I'll upload the diff-patch for you to evaluate.
Comment 3 Daniel Balog 2011-10-17 09:27:06 CEST
Created attachment 283 [details]
A patch that prevents a PBuffer from taking awtlocks during display

I uploaded a patch that modifies 4 files to make the given test succeed:

GLPBufferImpl
X11Util
GLContextImpl
X11GLXContext
Comment 4 Sven Gothel 2011-10-17 10:00:35 CEST
Thank you very much for your triage and proposal Daniel!

I definitely will look into this one before RC4, of course.

I like to check weather it's possible to apply your 'don't lock AWT in offscreen surface' logic
further below where we actually attach the lock object to the nativewindow graphics devices.
In general you are right, but applying it in the nativewindow layer would allow to make a more generic
approach.
Comment 5 Daniel Balog 2011-10-17 17:52:50 CEST
Yeah, I figured it wasn't general enough. The problem is that I get lost in the code real easily :)

Thanks for the update though, let me know if you need more help.
Comment 6 Sven Gothel 2011-11-23 04:27:51 CET
Ok .. I actually started to work on this issue today. 
Will update soon .. ie. revalidating the AWT lock and controlling it's use more fine grained.
This has to be done pretty carefully to not have regressions etc ..
Comment 7 Sven Gothel 2011-11-25 03:20:37 CET
Completed review of the underlying locking mechanism, commits:
  0bcf2c35e96cbf7a1e2537448f9603d7d487cf86
  e341ad8db546530b3a49c56c32cc26980e296201
  c84e235b3284d0e18481748b44594116e25821a9
  a94c1a2945a31aa5d93e354da3bc80f59f253ec4
  7ae0f2df39692e82d7955dbcd09c35c36382726c
  6b8f6e8d7c548cb6bfed14d8a04c9cf252ca7c4d
  9ef0a0c185ace5217efc014809e97c5eead842cc
  0314be79a7a93931a74fe4322bc78e699d7741e9

It was a good review, thanks to the bug report.

It resulted in a good cleanup of JAWTWindow usage and more, especially for X11.

The fixes didn't introduce any more states or complication
and actually removed a lot of them.

Offscreen surfaces (besides others) are now lock free, even on X11.