8339341: SurfaceManager cacheMap retains strong references

Reviewed-by: jdv, prr

Author: YaaZ

src/java.desktop/macosx/classes/sun/awt/CGraphicsConfig.java

@@ -31,20 +31,27 @@
 import java.awt.geom.AffineTransform;
 import java.awt.image.ColorModel;
 
+import sun.awt.image.SurfaceManager;
 import sun.java2d.SurfaceData;
 import sun.lwawt.LWGraphicsConfig;
 import sun.lwawt.macosx.CFRetainedResource;
 
 public abstract class CGraphicsConfig extends GraphicsConfiguration
-        implements LWGraphicsConfig {
+        implements LWGraphicsConfig, SurfaceManager.ProxiedGraphicsConfig {
 
     private final CGraphicsDevice device;
     private ColorModel colorModel;
+    private final SurfaceManager.ProxyCache surfaceDataProxyCache = new SurfaceManager.ProxyCache();
 
     protected CGraphicsConfig(CGraphicsDevice device) {
         this.device = device;
     }
 
+    @Override
+    public SurfaceManager.ProxyCache getSurfaceDataProxyCache() {
+        return surfaceDataProxyCache;
+    }
+
     @Override
     public final Rectangle getBounds() {
         return device.getBounds();

src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java

@@ -29,7 +29,6 @@
 import sun.awt.CGraphicsDevice;
 import sun.awt.image.OffScreenImage;
 import sun.awt.image.SunVolatileImage;
-import sun.awt.image.SurfaceManager;
 import sun.java2d.Disposer;
 import sun.java2d.DisposerRecord;
 import sun.java2d.Surface;
@@ -70,7 +69,7 @@
 import static sun.java2d.metal.MTLContext.MTLContextCaps.CAPS_EXT_BIOP_SHADER;
 
 public final class MTLGraphicsConfig extends CGraphicsConfig
-        implements AccelGraphicsConfig, SurfaceManager.ProxiedGraphicsConfig
+        implements AccelGraphicsConfig
 {
     private static ImageCapabilities imageCaps = new MTLImageCaps();
 
@@ -112,11 +111,6 @@ private MTLGraphicsConfig(CGraphicsDevice device,
                 new MTLGCDisposerRecord(pConfigInfo));
     }
 
-    @Override
-    public Object getProxyKey() {
-        return this;
-    }
-
     public SurfaceData createManagedSurface(int w, int h, int transparency) {
         return MTLSurfaceData.createData(this, w, h,
                 getColorModel(transparency),

src/java.desktop/macosx/classes/sun/java2d/metal/MTLSurfaceData.java

@@ -156,7 +156,7 @@ private MTLSurfaceData(MTLLayer layer, MTLGraphicsConfig gc,
         super(getCustomSurfaceType(type), cm);
         this.graphicsConfig = gc;
         this.type = type;
-        setBlitProxyKey(gc.getProxyKey());
+        setBlitProxyCache(gc.getSurfaceDataProxyCache());
 
         // TEXTURE shouldn't be scaled, it is used for managed BufferedImages.
         scale = type == TEXTURE ? 1 : gc.getDevice().getScaleFactor();

src/java.desktop/macosx/classes/sun/java2d/opengl/CGLGraphicsConfig.java

@@ -105,11 +105,6 @@ private CGLGraphicsConfig(CGraphicsDevice device, long configInfo,
                            new CGLGCDisposerRecord(pConfigInfo));
     }
 
-    @Override
-    public Object getProxyKey() {
-        return this;
-    }
-
     @Override
     public SurfaceData createManagedSurface(int w, int h, int transparency) {
         return CGLSurfaceData.createData(this, w, h,

src/java.desktop/share/classes/sun/awt/image/SurfaceManager.java

@@ -31,8 +31,11 @@
 import java.awt.ImageCapabilities;
 import java.awt.image.BufferedImage;
 import java.awt.image.VolatileImage;
+import java.lang.ref.WeakReference;
+import java.util.Collections;
 import java.util.Iterator;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.Map;
+import java.util.WeakHashMap;
 
 import sun.java2d.InvalidPipeException;
 import sun.java2d.SurfaceData;
@@ -89,47 +92,14 @@ public static void setManager(Image img, SurfaceManager mgr) {
         imgaccessor.setSurfaceManager(img, mgr);
     }
 
-    private volatile ConcurrentHashMap<Object,Object> cacheMap;
-
-    /**
-     * Return an arbitrary cached object for an arbitrary cache key.
-     * Other objects can use this mechanism to store cached data about
-     * the source image that will let them save time when using or
-     * manipulating the image in the future.
-     * <p>
-     * Note that the cache is maintained as a simple Map with no
-     * attempts to keep it up to date or invalidate it so any data
-     * stored here must either not be dependent on the state of the
-     * image or it must be individually tracked to see if it is
-     * outdated or obsolete.
-     * <p>
-     * The SurfaceData object of the primary (destination) surface
-     * has a StateTracker mechanism which can help track the validity
-     * and "currentness" of any data stored here.
-     * For convenience and expediency an object stored as cached
-     * data may implement the FlushableCacheData interface specified
-     * below so that it may be notified immediately if the flush()
-     * method is ever called.
-     */
-    public Object getCacheData(Object key) {
-        return (cacheMap == null) ? null : cacheMap.get(key);
-    }
-
     /**
-     * Store an arbitrary cached object for an arbitrary cache key.
-     * See the getCacheData() method for notes on tracking the
-     * validity of data stored using this mechanism.
+     * This map holds references to SurfaceDataProxy per given ProxyCache.
+     * Unlike ProxyCache, which contains SurfaceDataProxy objects per given SurfaceManager,
+     * this map does not prevent contained proxies from being garbage collected.
+     * Therefore, ProxyCache can be considered an "owning" container for the SurfaceDataProxy objects,
+     * and this map is just a weak mapping for the bookkeeping purposes.
      */
-    public void setCacheData(Object key, Object value) {
-        if (cacheMap == null) {
-            synchronized (this) {
-                if (cacheMap == null) {
-                    cacheMap = new ConcurrentHashMap<>(2);
-                }
-            }
-        }
-        cacheMap.put(key, value);
-    }
+    private final Map<ProxyCache, WeakReference<SurfaceDataProxy>> weakCache = new WeakHashMap<>(2);
 
     /**
      * Returns the main SurfaceData object that "owns" the pixels for
@@ -202,12 +172,10 @@ public boolean isAccelerated() {
                 tmpGc = GraphicsEnvironment.getLocalGraphicsEnvironment().
                     getDefaultScreenDevice().getDefaultConfiguration();
             }
-            if (tmpGc instanceof ProxiedGraphicsConfig) {
-                Object proxyKey =
-                    ((ProxiedGraphicsConfig) tmpGc).getProxyKey();
-                if (proxyKey != null) {
-                    SurfaceDataProxy sdp =
-                        (SurfaceDataProxy) getCacheData(proxyKey);
+            if (tmpGc instanceof ProxiedGraphicsConfig pgc) {
+                ProxyCache cache = pgc.getSurfaceDataProxyCache();
+                if (cache != null) {
+                    SurfaceDataProxy sdp = cache.get(SurfaceManager.this);
                     return (sdp != null && sdp.isAccelerated());
                 }
             }
@@ -222,13 +190,51 @@ public boolean isAccelerated() {
      * Implementing this interface facilitates the default
      * implementation of getImageCapabilities() above.
      */
-    public static interface ProxiedGraphicsConfig {
+    public interface ProxiedGraphicsConfig {
+
         /**
-         * Return the key that destination surfaces created on the
+         * Return the cache that destination surfaces created on the
          * given GraphicsConfiguration use to store SurfaceDataProxy
          * objects for their cached copies.
          */
-        public Object getProxyKey();
+        ProxyCache getSurfaceDataProxyCache();
+    }
+
+    public static class ProxyCache {
+        private final Map<SurfaceManager, SurfaceDataProxy> map = Collections.synchronizedMap(new WeakHashMap<>());
+
+        /**
+         * Return a cached SurfaceDataProxy object for a given SurfaceManager.
+         * <p>
+         * Note that the cache is maintained as a simple Map with no
+         * attempts to keep it up to date or invalidate it so any data
+         * stored here must either not be dependent on the state of the
+         * image or it must be individually tracked to see if it is
+         * outdated or obsolete.
+         * <p>
+         * The SurfaceData object of the primary (destination) surface
+         * has a StateTracker mechanism which can help track the validity
+         * and "currentness" of any data stored here.
+         * For convenience and expediency an object stored as cached
+         * data may implement the FlushableCacheData interface specified
+         * below so that it may be notified immediately if the flush()
+         * method is ever called.
+         */
+        public SurfaceDataProxy get(SurfaceManager manager) {
+            return map.get(manager);
+        }
+
+        /**
+         * Store a cached SurfaceDataProxy object for a given SurfaceManager.
+         * See the get() method for notes on tracking the
+         * validity of data stored using this mechanism.
+         */
+        public void put(SurfaceManager manager, SurfaceDataProxy proxy) {
+            synchronized (manager.weakCache) { // Synchronize on weakCache first!
+                manager.weakCache.put(this, new WeakReference<>(proxy));
+                map.put(manager, proxy);
+            }
+        }
     }
 
     /**
@@ -244,15 +250,13 @@ public synchronized void flush() {
         flush(false);
     }
 
-    synchronized void flush(boolean deaccelerate) {
-        if (cacheMap != null) {
-            Iterator<Object> i = cacheMap.values().iterator();
+    void flush(boolean deaccelerate) {
+        synchronized (weakCache) {
+            Iterator<WeakReference<SurfaceDataProxy>> i = weakCache.values().iterator();
             while (i.hasNext()) {
-                Object o = i.next();
-                if (o instanceof FlushableCacheData) {
-                    if (((FlushableCacheData) o).flush(deaccelerate)) {
-                        i.remove();
-                    }
+                SurfaceDataProxy sdp = i.next().get();
+                if (sdp == null || sdp.flush(deaccelerate)) {
+                    i.remove();
                 }
             }
         }

src/java.desktop/share/classes/sun/java2d/SurfaceData.java

@@ -113,7 +113,7 @@ public abstract class SurfaceData
 
     private static native void initIDs();
 
-    private Object blitProxyKey;
+    private SurfaceManager.ProxyCache blitProxyCache;
     private StateTrackableDelegate stateDelegate;
 
     static {
@@ -143,23 +143,21 @@ protected SurfaceData(State state) {
     }
 
     /**
-     * Subclasses can set a "blit proxy key" which will be used
-     * along with the SurfaceManager.getCacheData() mechanism to
+     * Subclasses can set a "blit proxy cache" which will be used
+     * along with the SurfaceManager to
      * store acceleration-compatible cached copies of source images.
-     * This key is a "tag" used to identify which cached copies
-     * are compatible with this destination SurfaceData.
-     * The getSourceSurfaceData() method uses this key to manage
-     * cached copies of a source image as described below.
+     * The getSourceSurfaceData() method uses this cache to manage
+     * copies of a source image as described below.
      * <p>
-     * The Object used as this key should be as unique as it needs
+     * The cache used should be as unique as it needs
      * to be to ensure that multiple acceleratible destinations can
-     * each store their cached copies separately under different keys
+     * each store their cached copies separately into different caches
      * without interfering with each other or getting back the wrong
      * cached copy.
      * <p>
-     * Many acceleratable SurfaceData objects can use their own
-     * GraphicsConfiguration as their proxy key as the GC object will
-     * typically be unique to a given screen and pixel format, but
+     * Many GraphicsConfiguration implementations have their own
+     * cache as the GC object is
+     * typically unique to a given screen and pixel format, but
      * other rendering destinations may have more or less stringent
      * sharing requirements.  For instance, X11 pixmaps can be
      * shared on a given screen by any GraphicsConfiguration that
@@ -168,14 +166,14 @@ protected SurfaceData(State state) {
      * a different cached proxy for each would be a waste.  One can
      * imagine platforms where a single cached copy can be created
      * and shared across all screens and pixel formats - such
-     * implementations could use a single heavily shared key Object.
+     * implementations could use a single heavily shared cache object.
      */
-    protected void setBlitProxyKey(Object key) {
-        // Caching is effectively disabled if we never have a proxy key
+    protected void setBlitProxyCache(SurfaceManager.ProxyCache cache) {
+        // Caching is effectively disabled if we never have a proxy cache
         // since the getSourceSurfaceData() method only does caching
-        // if the key is not null.
+        // if the cache is not null.
         if (SurfaceDataProxy.isCachingAllowed()) {
-            this.blitProxyKey = key;
+            this.blitProxyCache = cache;
         }
     }
 
@@ -192,7 +190,7 @@ protected void setBlitProxyKey(Object key) {
      * appropriate SurfaceDataProxy instance.
      * The parameters describe the type of imaging operation being performed.
      * <p>
-     * If a blitProxyKey was supplied by the subclass then it is
+     * If a blitProxyCache was supplied by the subclass then it is
      * used to potentially override the choice of source SurfaceData.
      * The outline of this process is:
      * <ol>
@@ -201,8 +199,8 @@ protected void setBlitProxyKey(Object key) {
      * <li> destSD gets the SurfaceManager of the source Image
      *      and first retrieves the default SD from it using
      *      getPrimarySurfaceData()
-     * <li> destSD uses its "blit proxy key" (if set) to look for
-     *      some cached data stored in the source SurfaceManager
+     * <li> destSD uses its "blit proxy cache" (if set) to look for
+     *      some cached data corresponding to the the source SurfaceManager
      * <li> If the cached data is null then makeProxyFor() is used
      *      to create some cached data which is stored back in the
      *      source SurfaceManager under the same key for future uses.
@@ -219,18 +217,15 @@ public SurfaceData getSourceSurfaceData(Image img,
     {
         SurfaceManager srcMgr = SurfaceManager.getManager(img);
         SurfaceData srcData = srcMgr.getPrimarySurfaceData();
-        if (img.getAccelerationPriority() > 0.0f &&
-            blitProxyKey != null)
-        {
-            SurfaceDataProxy sdp =
-                (SurfaceDataProxy) srcMgr.getCacheData(blitProxyKey);
+        if (img.getAccelerationPriority() > 0.0f && blitProxyCache != null) {
+            SurfaceDataProxy sdp = blitProxyCache.get(srcMgr);
             if (sdp == null || !sdp.isValid()) {
                 if (srcData.getState() == State.UNTRACKABLE) {
                     sdp = SurfaceDataProxy.UNCACHED;
                 } else {
                     sdp = makeProxyFor(srcData);
                 }
-                srcMgr.setCacheData(blitProxyKey, sdp);
+                blitProxyCache.put(srcMgr, sdp);
             }
             srcData = sdp.replaceData(srcData, txtype, comp, bgColor);
         }

src/java.desktop/share/classes/sun/java2d/opengl/OGLSurfaceData.java

@@ -232,7 +232,7 @@ protected OGLSurfaceData(OGLGraphicsConfig gc,
         super(getCustomSurfaceType(type), cm);
         this.graphicsConfig = gc;
         this.type = type;
-        setBlitProxyKey(gc.getProxyKey());
+        setBlitProxyCache(gc.getSurfaceDataProxyCache());
     }
 
     @Override

src/java.desktop/unix/classes/sun/awt/X11GraphicsConfig.java

@@ -180,8 +180,8 @@ public synchronized SurfaceType getSurfaceType() {
     }
 
     @Override
-    public Object getProxyKey() {
-        return device.getProxyKeyFor(getSurfaceType());
+    public SurfaceManager.ProxyCache getSurfaceDataProxyCache() {
+        return device.getProxyCacheFor(getSurfaceType());
     }
 
     /**