Ticket #13120: patch-fix-13120.patch

src/org/openstreetmap/josm/gui/layer/AbstractTileSourceLayer.java

@@ -89 +89 @@
 import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.io.WMSLayerImporter;
 import org.openstreetmap.josm.tools.GBC;
+import org.openstreetmap.josm.tools.MemoryManager;
+import org.openstreetmap.josm.tools.MemoryManager.MemoryHandle;
+import org.openstreetmap.josm.tools.MemoryManager.NotEnoughMemoryException;
 
 /**
  * Base abstract class that supports displaying images provided by TileSource. It might be TMS source, WMS or WMTS
@@ -693 +696 @@
             if (tileSource == null) {
                 throw new IllegalArgumentException(tr("Failed to create tile source"));
             }
-            checkLayerMemoryDoesNotExceedMaximum();
             // check if projection is supported
             projectionChanged(null, Main.getProjection());
             initTileSource(this.tileSource);
@@ -702 +704 @@
 
     @Override
     protected LayerPainter createMapViewPainter(MapViewEvent event) {
-        return new CompatibilityModeLayerPainter() {
-            @Override
-            public void detachFromMapView(MapViewEvent event) {
-                event.getMapView().removeMouseListener(adapter);
-                MapView.removeZoomChangeListener(AbstractTileSourceLayer.this);
-                super.detachFromMapView(event);
-            }
-        };
+        return new TileSourcePainter();
     }
 
     /**
@@ -733 +728 @@
         }
     }
 
-    @Override
-    protected long estimateMemoryUsage() {
-        return 4L * tileSource.getTileSize() * tileSource.getTileSize() * estimateTileCacheSize();
-    }
-
     protected int estimateTileCacheSize() {
         Dimension screenSize = GuiHelper.getMaximumScreenSize();
         int height = screenSize.height;
@@ -1939 +1929 @@
         super.destroy();
         adjustAction.destroy();
     }
+
+    private class TileSourcePainter extends CompatibilityModeLayerPainter {
+        /**
+         * The memory handle that will hold our tile source.
+         */
+        private MemoryHandle<?> memory;
+
+        @Override
+        public void paint(MapViewGraphics graphics) {
+            allocateCacheMemory();
+            if (memory != null) {
+                super.paint(graphics);
+            }
+        }
+
+        private void allocateCacheMemory() {
+            if (memory == null) {
+                MemoryManager manager = MemoryManager.getInstance();
+                if (manager.isAvailable(getEstimatedCacheSize())) {
+                    try {
+                        memory = manager.allocateMemory("tile source layer", getEstimatedCacheSize(), () -> new Object());
+                    } catch (NotEnoughMemoryException e) {
+                        Main.warn("Could not allocate tile source memory", e);
+                    }
+                }
+            }
+        }
+
+        protected long getEstimatedCacheSize() {
+            return 4L * tileSource.getTileSize() * tileSource.getTileSize() * estimateTileCacheSize();
+        }
+
+        @Override
+        public void detachFromMapView(MapViewEvent event) {
+            event.getMapView().removeMouseListener(adapter);
+            MapView.removeZoomChangeListener(AbstractTileSourceLayer.this);
+            super.detachFromMapView(event);
+            if (memory != null) {
+                memory.free();
+            }
+        }
+    }
 }

src/org/openstreetmap/josm/gui/layer/Layer.java

@@ -158 +158 @@
      * It is always called in the event dispatching thread.
      * Note that Main.map is null as long as no layer has been added, so do
      * not execute code in the constructor, that assumes Main.map.mapView is
-     * not null. Instead override this method.
+     * not null.
      *
-     * This implementation provides check, if JOSM will be able to use Layer. Layers
-     * using a lot of memory, which do know in advance, how much memory they use, should
-     * override {@link #estimateMemoryUsage() estimateMemoryUsage} method and give a hint.
-     *
-     * This allows for preemptive warning message for user, instead of failing later on
-     *
-     * Remember to call {@code super.hookUpMapView()} when overriding this method
+     * If you need to execute code when this layer is added to the map view, use
+     * {@link #attachToMapView(org.openstreetmap.josm.gui.layer.MapViewPaintable.MapViewEvent)}
      */
     public void hookUpMapView() {
-        checkLayerMemoryDoesNotExceedMaximum();
-    }
-
-    /**
-     * Checks that the memory required for the layers is no greather than the max memory.
-     */
-    protected static void checkLayerMemoryDoesNotExceedMaximum() {
-        // calculate total memory needed for all layers
-        long memoryBytesRequired = 50L * 1024L * 1024L; // assumed minimum JOSM memory footprint
-        for (Layer layer: Main.getLayerManager().getLayers()) {
-            memoryBytesRequired += layer.estimateMemoryUsage();
-        }
-        if (memoryBytesRequired > Runtime.getRuntime().maxMemory()) {
-            throw new IllegalArgumentException(
-                    tr("To add another layer you need to allocate at least {0,number,#}MB memory to JOSM using -Xmx{0,number,#}M "
-                            + "option (see http://forum.openstreetmap.org/viewtopic.php?id=25677).\n"
-                            + "Currently you have {1,number,#}MB memory allocated for JOSM",
-                            memoryBytesRequired / 1024 / 1024, Runtime.getRuntime().maxMemory() / 1024 / 1024));
-        }
     }
 
     /**
@@ -588 +564 @@
 
     /**
      * @return bytes that the tile will use. Needed for resource management
+     * @deprecated Not used any more.
      */
+    @Deprecated
     protected long estimateMemoryUsage() {
         return 0;
     }

new file src/org/openstreetmap/josm/tools/MemoryManager.java

@@ +1 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.tools;
+import static org.openstreetmap.josm.tools.I18n.tr;
+
+import java.text.MessageFormat;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.Supplier;
+
+import org.openstreetmap.josm.Main;
+
+/**
+ * This class allows all components of JOSM to register reclaimable amounts to memory.
+ * <p>
+ * It can be used to hold imagery caches or other data that can be reconstructed form disk/web if required.
+ * <p>
+ * Reclaimable storage implementations may be added in the future.
+ *
+ * @author Michael Zangl
+ * @since xxx
+ */
+public class MemoryManager {
+    /**
+     * assumed minimum JOSM memory footprint
+     */
+    private static final long JOSM_CORE_FOOTPRINT = 50L * 1024L * 1024L;
+
+    private static final MemoryManager INSTANCE = new MemoryManager();
+
+    private ArrayList<MemoryHandle<?>> activeHandles = new ArrayList<>();
+
+    protected MemoryManager() {
+    }
+
+    /**
+     * Allocates a basic, fixed memory size.
+     * <p>
+     * If there is enough free memory, the factory is used to procude one element which is then returned as memory handle.
+     * <p>
+     * You should invoke {@link MemoryHandle#free()} if you do not need that handle any more.
+     * @param <T> The content type of the memory-
+     * @param name A name for the memory area. Only used for debugging.
+     * @param maxBytes The maximum amount of bytes the content may have
+     * @param factory The factory to use to procude the content if there is sufficient memory.
+     * @return A memory handle to the content.
+     * @throws NotEnoughMemoryException If there is not enough memory to allocate.
+     */
+    public synchronized <T> MemoryHandle<T> allocateMemory(String name, long maxBytes, Supplier<T> factory) throws NotEnoughMemoryException {
+        if (isAvailable(maxBytes)) {
+            T content = factory.get();
+            if (content == null) {
+                throw new IllegalArgumentException("Factory did not return a content element.");
+            }
+            Main.info(MessageFormat.format("Allocate for {0}: {1} MB of memory. Available: {2} MB.",
+                    name, maxBytes / 1024 / 1024, getAvailableMemory() / 1024 / 1024));
+            MemoryHandle<T> handle = new ManualFreeMemoryHandle<>(name, content, maxBytes);
+            activeHandles.add(handle);
+            return handle;
+        } else {
+            throw new NotEnoughMemoryException(maxBytes);
+        }
+    }
+
+    /**
+     * Check if that memory is available
+     * @param maxBytes The memory to check for
+     * @return <code>true</code> if that memory is available.
+     */
+    public synchronized boolean isAvailable(long maxBytes) {
+        if (maxBytes < 0) {
+            throw new IllegalArgumentException(MessageFormat.format("Cannot allocate negative number of bytes: {0]", maxBytes));
+        }
+        return getAvailableMemory() >= maxBytes;
+    }
+
+    /**
+     * Gets the maximum amount of memory available for use in this manager.
+     * @return The maximum amount of memory.
+     */
+    public synchronized long getMaxMemory() {
+        return Runtime.getRuntime().maxMemory() - JOSM_CORE_FOOTPRINT;
+    }
+
+    /**
+     * Gets the memory that is considered free.
+     * @return The memory that can be used for new allocations.
+     */
+    public synchronized long getAvailableMemory() {
+        return getMaxMemory() - activeHandles.stream().mapToLong(h -> h.getSize()).sum();
+    }
+
+    /**
+     * Get the global memory manager instance.
+     * @return The memory manager.
+     */
+    public static MemoryManager getInstance() {
+        return INSTANCE;
+    }
+
+    /**
+     * Reset the state of this manager to the default state.
+     * @return true if there were entries that have been reset.
+     */
+    protected synchronized List<MemoryHandle<?>> resetState() {
+        ArrayList<MemoryHandle<?>> toFree = new ArrayList<>(activeHandles);
+        toFree.stream().forEach(h -> h.free());
+        return toFree;
+    }
+
+    /**
+     * A memory area managed by the {@link MemoryManager}.
+     * @author Michael Zangl
+     * @param <T> The content type.
+     * @since xxx
+     */
+    public interface MemoryHandle<T> {
+
+        /**
+         * Gets the content of this memory area.
+         * <p>
+         * This method should be the prefered access to the memory since it will do error checking when {@link #free()} was called.
+         * @return The memory area content.
+         */
+        public T get();
+
+        /**
+         * Get the size that was requested for this memory area.
+         * @return the size
+         */
+        public long getSize();
+
+        /**
+         * Manually release this memory area. There should be no memory consumed by this afterwards.
+         */
+        public void free();
+    }
+
+    private class ManualFreeMemoryHandle<T> implements MemoryHandle<T> {
+        private final String name;
+        private T content;
+        private final long size;
+
+        ManualFreeMemoryHandle(String name, T content, long size) {
+            this.name = name;
+            this.content = content;
+            this.size = size;
+        }
+
+        @Override
+        public T get() {
+            if (content == null) {
+                throw new IllegalStateException(MessageFormat.format("Memory area was accessed after free(): {0}", name));
+            }
+            return content;
+        }
+
+        @Override
+        public long getSize() {
+            return size;
+        }
+
+        @Override
+        public void free() {
+            if (content == null) {
+                throw new IllegalStateException(MessageFormat.format("Memory area was already marked as freed: {0}", name));
+            }
+            content = null;
+            synchronized (MemoryManager.this) {
+                activeHandles.remove(this);
+            }
+        }
+
+        @Override
+        public String toString() {
+            return "MemoryHandle [name=" + name + ", size=" + size + "]";
+        }
+    }
+
+    /**
+     * This exception is thrown if there is not enough memory for allocating the given object.
+     * @author Michael Zangl
+     * @since xxx
+     */
+    public static class NotEnoughMemoryException extends Exception {
+        NotEnoughMemoryException(long memoryBytesRequired) {
+            super(tr("To add another layer you need to allocate at least {0,number,#}MB memory to JOSM using -Xmx{0,number,#}M "
+                            + "option (see http://forum.openstreetmap.org/viewtopic.php?id=25677).\n"
+                            + "Currently you have {1,number,#}MB memory allocated for JOSM",
+                            memoryBytesRequired / 1024 / 1024, Runtime.getRuntime().maxMemory() / 1024 / 1024));
+        }
+    }
+}

test/unit/org/openstreetmap/josm/testutils/JOSMTestRules.java

@@ -18 +18 @@
 import org.openstreetmap.josm.io.OsmApiInitializationException;
 import org.openstreetmap.josm.io.OsmTransferCanceledException;
 import org.openstreetmap.josm.tools.I18n;
+import org.openstreetmap.josm.tools.MemoryManagerTest;
 import org.openstreetmap.josm.tools.date.DateUtils;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
@@ -38 +39 @@
     private String i18n = null;
     private boolean platform;
     private boolean useProjection;
+    private boolean allowMemoryManagerLeaks;
 
     /**
      * Disable the default timeout for this test. Use with care.
@@ -132 +134 @@
         return this;
     }
 
+    /**
+     * Allow the memory manager to contain items after execution of the test cases.
+     * @return this instance, for easy chaining
+     */
+    public JOSMTestRules memoryManagerLeaks() {
+        allowMemoryManagerLeaks = true;
+        return this;
+    }
+
     @Override
     public Statement apply(Statement base, Description description) {
         Statement statement = base;
@@ -217 +228 @@
      */
     @SuppressFBWarnings("DM_GC")
     private void cleanUpFromJosmFixture() {
+        MemoryManagerTest.resetState(true);
         Main.getLayerManager().resetState();
         Main.pref = null;
         Main.platform = null;
@@ -236 +248 @@
         });
         // Remove all layers
         Main.getLayerManager().resetState();
+        MemoryManagerTest.resetState(allowMemoryManagerLeaks);
 
         // TODO: Remove global listeners and other global state.
         Main.pref = null;

new file test/unit/org/openstreetmap/josm/tools/MemoryManagerTest.java

@@ +1 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.tools;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.util.List;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
+import org.openstreetmap.josm.tools.MemoryManager.MemoryHandle;
+import org.openstreetmap.josm.tools.MemoryManager.NotEnoughMemoryException;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+/**
+ * Tests the {@link MemoryManager} class.
+ * @author Michael Zangl
+ * @since xxx
+ */
+public class MemoryManagerTest {
+    /**
+     * Base test environment
+     */
+    @Rule
+    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
+    public JOSMTestRules test = new JOSMTestRules().memoryManagerLeaks();
+
+    /**
+     * Test {@link MemoryManager#allocateMemory(String, long, java.util.function.Supplier)}
+     * @throws NotEnoughMemoryException
+     */
+    @Test
+    public void testUseMemory() throws NotEnoughMemoryException {
+        MemoryManager manager = MemoryManager.getInstance();
+        long available = manager.getAvailableMemory();
+        assertTrue(available < Runtime.getRuntime().maxMemory());
+        assertEquals(available, manager.getMaxMemory());
+
+        Object o1 = new Object();
+        MemoryHandle<Object> testMemory = manager.allocateMemory("test", 10, () -> o1);
+        assertEquals(available - 10, manager.getAvailableMemory());
+        assertSame(o1, testMemory.get());
+        assertEquals(10, testMemory.getSize());
+        assertTrue(testMemory.toString().startsWith("MemoryHandle"));
+
+        manager.allocateMemory("test2", 10, () -> new Object());
+        assertEquals(available - 20, manager.getAvailableMemory());
+
+        testMemory.free();
+        assertEquals(available - 10, manager.getAvailableMemory());
+    }
+
+    /**
+     * Test that {@link MemoryHandle#get()} checks for use after free.
+     * @throws NotEnoughMemoryException
+     */
+    @Test(expected = IllegalStateException.class)
+    public void testUseAfterFree() throws NotEnoughMemoryException {
+        MemoryManager manager = MemoryManager.getInstance();
+        MemoryHandle<Object> testMemory = manager.allocateMemory("test", 10, () -> new Object());
+        testMemory.free();
+        testMemory.get();
+    }
+
+    /**
+     * Test that {@link MemoryHandle#get()} checks for free after free.
+     * @throws NotEnoughMemoryException
+     */
+    @Test(expected = IllegalStateException.class)
+    public void testFreeAfterFree() throws NotEnoughMemoryException {
+        MemoryManager manager = MemoryManager.getInstance();
+        MemoryHandle<Object> testMemory = manager.allocateMemory("test", 10, () -> new Object());
+        testMemory.free();
+        testMemory.free();
+    }
+    /**
+     * Test that too big allocations fail
+     * @throws NotEnoughMemoryException
+     */
+    @Test(expected = NotEnoughMemoryException.class)
+    public void testAllocationFails() throws NotEnoughMemoryException {
+        MemoryManager manager = MemoryManager.getInstance();
+        long available = manager.getAvailableMemory();
+
+        manager.allocateMemory("test", available + 1, () -> {
+            fail("Should not reach");
+            return null;
+        });
+    }
+
+    /**
+     * Test that allocations with null object fail
+     * @throws NotEnoughMemoryException
+     */
+    @Test(expected = IllegalArgumentException.class)
+    public void testSupplierFails() throws NotEnoughMemoryException {
+        MemoryManager manager = MemoryManager.getInstance();
+
+        manager.allocateMemory("test", 1, () -> null);
+    }
+
+    /**
+     * Test {@link MemoryManager#isAvailable(long)}
+     */
+    @Test
+    public void testIsAvailable() {
+        MemoryManager manager = MemoryManager.getInstance();
+        assertTrue(manager.isAvailable(10));
+        assertTrue(manager.isAvailable(100));
+        assertTrue(manager.isAvailable(10));
+    }
+
+    /**
+     * Test {@link MemoryManager#isAvailable(long)} for negative number
+     * @throws NotEnoughMemoryException
+     */
+    @Test(expected = IllegalArgumentException.class)
+    public void testIsAvailableFails() throws NotEnoughMemoryException {
+        MemoryManager manager = MemoryManager.getInstance();
+
+        manager.isAvailable(-10);
+    }
+
+    /**
+     * Test {@link MemoryManager#resetState()}
+     * @throws NotEnoughMemoryException
+     */
+    @Test
+    public void testResetState() throws NotEnoughMemoryException {
+        MemoryManager manager = MemoryManager.getInstance();
+        assertTrue(manager.resetState().isEmpty());
+
+        manager.allocateMemory("test", 10, () -> new Object());
+        manager.allocateMemory("test2", 10, () -> new Object());
+        assertEquals(2, manager.resetState().size());
+
+        assertTrue(manager.resetState().isEmpty());
+    }
+
+    /**
+     * Test {@link MemoryManager#resetState()}
+     * @throws NotEnoughMemoryException
+     */
+    @Test(expected = IllegalStateException.class)
+    public void testResetStateUseAfterFree() throws NotEnoughMemoryException {
+        MemoryManager manager = MemoryManager.getInstance();
+        MemoryHandle<Object> testMemory = manager.allocateMemory("test", 10, () -> new Object());
+
+        assertFalse(manager.resetState().isEmpty());
+        testMemory.get();
+    }
+
+    /**
+     * Reset the state of the memory manager
+     * @param allowMemoryManagerLeaks If this is set, no exception is thrown if there were leaking entries.
+     */
+    public static void resetState(boolean allowMemoryManagerLeaks) {
+        List<MemoryHandle<?>> hadLeaks = MemoryManager.getInstance().resetState();
+        if (!allowMemoryManagerLeaks) {
+            assertTrue("Memory manager had leaking memory: " + hadLeaks, hadLeaks.isEmpty());
+        }
+    }
+}