Bug 519

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.

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.

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

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.

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.

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 ..

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.