Subject: [PATCH] Fix #20141: ImageProvider: cache rendered SVG images using JCS --- Index: src/org/openstreetmap/josm/data/cache/JCSCacheManager.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/data/cache/JCSCacheManager.java b/src/org/openstreetmap/josm/data/cache/JCSCacheManager.java --- a/src/org/openstreetmap/josm/data/cache/JCSCacheManager.java (revision 18755) +++ b/src/org/openstreetmap/josm/data/cache/JCSCacheManager.java (date 1686748970335) @@ -30,6 +30,7 @@ import org.openstreetmap.josm.data.preferences.BooleanProperty; import org.openstreetmap.josm.data.preferences.IntegerProperty; import org.openstreetmap.josm.spi.preferences.Config; +import org.openstreetmap.josm.tools.ImageResource; import org.openstreetmap.josm.tools.Logging; import org.openstreetmap.josm.tools.Utils; @@ -49,6 +50,12 @@ */ public static final BooleanProperty USE_BLOCK_CACHE = new BooleanProperty(PREFERENCE_PREFIX + ".use_block_cache", true); + /** + * The preference key {@code jcs.cache.use_image_resource_cache} controls the caching mechanism used for {@link ImageResource}. + * If set to {@code true}, a combined memory/disk is used. Otherwise, an in-memory-cache is used. + */ + public static final BooleanProperty USE_IMAGE_RESOURCE_CACHE = new BooleanProperty(PREFERENCE_PREFIX + ".use_image_resource_cache", false); + private static final AuxiliaryCacheFactory DISK_CACHE_FACTORY = getDiskCacheFactory(); private static FileLock cacheDirLock; @@ -173,7 +180,7 @@ * @return cache access object */ public static CacheAccess getCache(String cacheName) { - return getCache(cacheName, DEFAULT_MAX_OBJECTS_IN_MEMORY.get().intValue(), 0, null); + return getCache(cacheName, DEFAULT_MAX_OBJECTS_IN_MEMORY.get(), 0, null); } /** @@ -187,12 +194,19 @@ * @return cache access object */ public static CacheAccess getCache(String cacheName, int maxMemoryObjects, int maxDiskObjects, String cachePath) { + // TODO Get block size in Java 9: Files.getFileStore(java.nio.file.Paths.get(cachePath)).getBlockSize(); + return getCache(cacheName, maxMemoryObjects, maxDiskObjects, cachePath, + Boolean.TRUE.equals(USE_BLOCK_CACHE.get()) ? 4096 : 0); + } + + private static CacheAccess getCache(String cacheName, int maxMemoryObjects, int maxDiskObjects, + String cachePath, int blockSizeBytes) { CacheAccess cacheAccess = getCacheAccess(cacheName, getCacheAttributes(maxMemoryObjects)); if (cachePath != null && cacheDirLock != null && cacheAccess != null && DISK_CACHE_FACTORY != null) { CompositeCache cc = cacheAccess.getCacheControl(); try { - IDiskCacheAttributes diskAttributes = getDiskCacheAttributes(maxDiskObjects, cachePath, cacheName); + IDiskCacheAttributes diskAttributes = getDiskCacheAttributes(maxDiskObjects, cachePath, cacheName, blockSizeBytes); if (cc.getAuxCacheList().isEmpty()) { cc.setAuxCaches(Collections.singletonList(DISK_CACHE_FACTORY.createCache( diskAttributes, null, null, new StandardSerializer()))); @@ -215,6 +229,21 @@ } } + /** + * Returns a cache for {@link ImageResource} + * @param key type + * @param value type + * @return cache access object + */ + public static CacheAccess getImageResourceCache() { + if (Boolean.FALSE.equals(USE_IMAGE_RESOURCE_CACHE.get())) { + return getCache("images", 16 * 1024, 0, null); + } + String cachePath = new File(Config.getDirs().getCacheDirectory(true), "images").getAbsolutePath(); + Logging.warn("Using experimental disk cache {0} for ImageResource", cachePath); + return getCache("images", 16 * 1024, 512 * 1024, cachePath, 1024); + } + /** * Close all files to ensure, that all indexes and data are properly written */ @@ -222,12 +251,13 @@ JCS.shutdown(); } - private static IDiskCacheAttributes getDiskCacheAttributes(int maxDiskObjects, String cachePath, String cacheName) { + private static IDiskCacheAttributes getDiskCacheAttributes(int maxDiskObjects, String cachePath, String cacheName, int blockSizeBytes) { IDiskCacheAttributes ret; - removeStaleFiles(cachePath + File.separator + cacheName, useBlockCache() ? "_INDEX_v2" : "_BLOCK_v2"); - String newCacheName = cacheName + (useBlockCache() ? "_BLOCK_v2" : "_INDEX_v2"); + boolean isBlockDiskCache = blockSizeBytes > 0 && useBlockCache(); + removeStaleFiles(cachePath + File.separator + cacheName, isBlockDiskCache ? "_INDEX_v2" : "_BLOCK_v2"); + String newCacheName = cacheName + (isBlockDiskCache ? "_BLOCK_v2" : "_INDEX_v2"); - if (useBlockCache()) { + if (isBlockDiskCache) { BlockDiskCacheAttributes blockAttr = new BlockDiskCacheAttributes(); /* * BlockDiskCache never optimizes the file, so when file size is reduced, it will never be truncated to desired size. @@ -241,7 +271,7 @@ } else { blockAttr.setMaxKeySize(maxDiskObjects); } - blockAttr.setBlockSizeBytes(4096); // use 4k blocks + blockAttr.setBlockSizeBytes(blockSizeBytes); ret = blockAttr; } else { IndexedDiskCacheAttributes indexAttr = new IndexedDiskCacheAttributes(); Index: src/org/openstreetmap/josm/gui/MainInitialization.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/gui/MainInitialization.java b/src/org/openstreetmap/josm/gui/MainInitialization.java --- a/src/org/openstreetmap/josm/gui/MainInitialization.java (revision 18755) +++ b/src/org/openstreetmap/josm/gui/MainInitialization.java (date 1686746143350) @@ -152,6 +152,8 @@ MainApplication.toolbar.refreshToolbarControl(); MainApplication.toolbar.control.updateUI(); MainApplication.contentPanePrivate.updateUI(); + // image provider statistics + ImageProvider.printStatistics(); })) ); } Index: src/org/openstreetmap/josm/tools/ImageProvider.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/tools/ImageProvider.java b/src/org/openstreetmap/josm/tools/ImageProvider.java --- a/src/org/openstreetmap/josm/tools/ImageProvider.java (revision 18755) +++ b/src/org/openstreetmap/josm/tools/ImageProvider.java (date 1686746143352) @@ -930,7 +930,7 @@ if (path == null) { continue; } - ir = getIfAvailableLocalURL(path, type); + ir = getIfAvailableLocalURL(subdir + name, path, type); if (ir != null) { cache.put(cacheName, ir); return ir; @@ -960,7 +960,7 @@ URI uri = getSvgUniverse().loadSVG(is, Utils.fileToURL(cf.getFile()).toString()); svg = getSvgUniverse().getDiagram(uri); } - return svg == null ? null : new ImageResource(svg); + return svg == null ? null : new ImageResource(url, svg); case OTHER: BufferedImage img = null; try { @@ -968,7 +968,7 @@ } catch (IOException | UnsatisfiedLinkError e) { Logging.log(Logging.LEVEL_WARN, "Exception while reading HTTP image:", e); } - return img == null ? null : new ImageResource(img); + return img == null ? null : new ImageResource(url, img); default: throw new AssertionError("Unsupported type: " + type); } @@ -1017,7 +1017,7 @@ Logging.warn("Unable to process svg: "+s); return null; } - return new ImageResource(svg); + return new ImageResource(url, svg); } else { try { // See #10479: for PNG files, always enforce transparency to be sure tNRS chunk is used even not in paletted mode @@ -1026,7 +1026,7 @@ // hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/dc4322602480/src/share/classes/com/sun/imageio/plugins/png/PNGImageReader.java#l656 // CHECKSTYLE.ON: LineLength Image img = read(new ByteArrayInputStream(bytes), false, true); - return img == null ? null : new ImageResource(img); + return img == null ? null : new ImageResource(url, img); } catch (IOException | UnsatisfiedLinkError e) { Logging.log(Logging.LEVEL_WARN, "Exception while reading image:", e); } @@ -1101,7 +1101,7 @@ URI uri = getSvgUniverse().loadSVG(is, entryName, true); svg = getSvgUniverse().getDiagram(uri); } - return svg == null ? null : new ImageResource(svg); + return svg == null ? null : new ImageResource(fullName, svg); case OTHER: while (size > 0) { int l = is.read(buf, offs, size); @@ -1114,7 +1114,7 @@ } catch (IOException | UnsatisfiedLinkError e) { Logging.warn(e); } - return img == null ? null : new ImageResource(img); + return img == null ? null : new ImageResource(fullName, img); default: throw new AssertionError("Unknown ImageType: "+type); } @@ -1133,31 +1133,32 @@ * @param type data type of the image * @return the requested image or null if the request failed */ - private static ImageResource getIfAvailableLocalURL(URL path, ImageType type) { + private static ImageResource getIfAvailableLocalURL(String cacheKey, URL path, ImageType type) { switch (type) { case SVG: - SVGDiagram svg = null; - synchronized (getSvgUniverse()) { - try { - URI uri = null; - try { - uri = getSvgUniverse().loadSVG(path); - } catch (InvalidPathException e) { - Logging.error("Cannot open {0}: {1}", path, e.getMessage()); - Logging.trace(e); - } - if (uri == null && "jar".equals(path.getProtocol())) { - URL betterPath = Utils.betterJarUrl(path); - if (betterPath != null) { - uri = getSvgUniverse().loadSVG(betterPath); - } - } - svg = getSvgUniverse().getDiagram(uri); - } catch (SecurityException | IOException e) { - Logging.log(Logging.LEVEL_WARN, "Unable to read SVG", e); - } - } - return svg == null ? null : new ImageResource(svg); + return new ImageResource(cacheKey, () -> { + synchronized (getSvgUniverse()) { + try { + URI uri = null; + try { + uri = getSvgUniverse().loadSVG(path); + } catch (InvalidPathException e) { + Logging.error("Cannot open {0}: {1}", path, e.getMessage()); + Logging.trace(e); + } + if (uri == null && "jar".equals(path.getProtocol())) { + URL betterPath = Utils.betterJarUrl(path); + if (betterPath != null) { + uri = getSvgUniverse().loadSVG(betterPath); + } + } + return getSvgUniverse().getDiagram(uri); + } catch (SecurityException | IOException e) { + Logging.log(Logging.LEVEL_WARN, "Unable to read SVG", e); + } + } + return null; + }); case OTHER: BufferedImage img = null; try { @@ -1172,7 +1173,7 @@ Logging.log(Logging.LEVEL_WARN, "Unable to read image", e); Logging.debug(e); } - return img == null ? null : new ImageResource(img); + return img == null ? null : new ImageResource(path.toString(), img); default: throw new AssertionError(); } @@ -1370,7 +1371,7 @@ * @since 6172 */ public static Image createBoundedImage(Image img, int maxSize) { - return new ImageResource(img).getImageIconBounded(new Dimension(maxSize, maxSize)).getImage(); + return new ImageResource(img.toString(), img).getImageIconBounded(new Dimension(maxSize, maxSize)).getImage(); } /** @@ -1952,6 +1953,14 @@ return new ImageIcon(new BufferedImage(size.getAdjustedWidth(), size.getAdjustedHeight(), BufferedImage.TYPE_INT_ARGB)); } + /** + * Prints statistics concerning the image loading caches. + */ + public static void printStatistics() { + Logging.info(getSvgUniverse().statistics()); + Logging.info(ImageResource.statistics()); + } + @Override public String toString() { return ("ImageProvider [" Index: src/org/openstreetmap/josm/tools/ImageResource.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/src/org/openstreetmap/josm/tools/ImageResource.java b/src/org/openstreetmap/josm/tools/ImageResource.java --- a/src/org/openstreetmap/josm/tools/ImageResource.java (revision 18755) +++ b/src/org/openstreetmap/josm/tools/ImageResource.java (date 1686746585943) @@ -4,9 +4,12 @@ import java.awt.Dimension; import java.awt.Image; import java.awt.image.BufferedImage; +import java.io.IOException; +import java.io.UncheckedIOException; import java.util.List; -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; +import java.util.Locale; +import java.util.Objects; +import java.util.function.Supplier; import javax.swing.AbstractAction; import javax.swing.Action; @@ -15,6 +18,9 @@ import javax.swing.JPanel; import javax.swing.UIManager; +import org.apache.commons.jcs3.access.behavior.ICacheAccess; +import org.openstreetmap.josm.data.cache.BufferedImageCacheEntry; +import org.openstreetmap.josm.data.cache.JCSCacheManager; import org.openstreetmap.josm.tools.ImageProvider.ImageSizes; import com.kitfox.svg.SVGDiagram; @@ -30,14 +36,21 @@ public class ImageResource { /** - * Caches the image data for resized versions of the same image. The key is obtained using {@link ImageResizeMode#cacheKey(Dimension)}. + * Caches the image data for resized versions of the same image. + * Depending on {@link JCSCacheManager#USE_IMAGE_RESOURCE_CACHE}, a combined memory/disk, or an in-memory-cache is used. */ - private final Map imgCache = new ConcurrentHashMap<>(4); + private static final ICacheAccess imgCache = JCSCacheManager.getImageResourceCache(); + + private final String cacheKey; /** * SVG diagram information in case of SVG vector image. */ private SVGDiagram svg; /** + * Supplier for SVG diagram information in case of possibly cached SVG vector image. + */ + private Supplier svgSupplier; + /** * Use this dimension to request original file dimension. */ public static final Dimension DEFAULT_DIMENSION = new Dimension(-1, -1); @@ -56,20 +69,27 @@ /** * Constructs a new {@code ImageResource} from an image. + * @param cacheKey the caching identifier of the image * @param img the image */ - public ImageResource(Image img) { - CheckParameterUtil.ensureParameterNotNull(img); - baseImage = img; + public ImageResource(String cacheKey, Image img) { + this.cacheKey = Objects.requireNonNull(cacheKey); + this.baseImage = Objects.requireNonNull(img); } /** * Constructs a new {@code ImageResource} from SVG data. + * @param cacheKey the caching identifier of the image * @param svg SVG data */ - public ImageResource(SVGDiagram svg) { - CheckParameterUtil.ensureParameterNotNull(svg); - this.svg = svg; + public ImageResource(String cacheKey, SVGDiagram svg) { + this.cacheKey = Objects.requireNonNull(cacheKey); + this.svg = Objects.requireNonNull(svg); + } + + public ImageResource(String cacheKey, Supplier svgSupplier) { + this.cacheKey = Objects.requireNonNull(cacheKey); + this.svgSupplier = Objects.requireNonNull(svgSupplier); } /** @@ -79,7 +99,9 @@ * @since 8095 */ public ImageResource(ImageResource res, List overlayInfo) { + this.cacheKey = res.cacheKey; this.svg = res.svg; + this.svgSupplier = res.svgSupplier; this.baseImage = res.baseImage; this.overlayInfo = overlayInfo; } @@ -165,6 +187,19 @@ return getImageIconAlreadyScaled(GuiSizesHelper.getDimensionDpiAdjusted(dim), multiResolution, false, resizeMode); } + private BufferedImage getImageFromCache(String cacheKey) { + try { + BufferedImageCacheEntry image = imgCache.get(cacheKey); + if (image == null || image.getImage() == null) { + return null; + } + Logging.trace("{0} is in cache :-)", cacheKey); + return image.getImage(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + /** * Get an ImageIcon object for the image of this resource. A potential UI scaling is assumed * to be already taken care of, so dim is already scaled accordingly. @@ -187,9 +222,15 @@ } else if (resizeMode == null) { resizeMode = ImageResizeMode.BOUNDED; } - final int cacheKey = resizeMode.cacheKey(dim); - BufferedImage img = imgCache.get(cacheKey); + final String cacheKey = String.format(Locale.ROOT, "%s--%s--%d--%d", + this.cacheKey, resizeMode.name(), dim.width, dim.height); + BufferedImage img = getImageFromCache(cacheKey); if (img == null) { + if (svgSupplier != null) { + svg = svgSupplier.get(); + Logging.trace("{0} is not in cache :-(", cacheKey); + svgSupplier = null; + } if (svg != null) { img = ImageProvider.createImageFromSvg(svg, dim, resizeMode); if (img == null) { @@ -221,7 +262,12 @@ img = new BufferedImage(img.getWidth(), img.getHeight(), BufferedImage.TYPE_4BYTE_ABGR); disabledIcon.paintIcon(new JPanel(), img.getGraphics(), 0, 0); } - imgCache.put(cacheKey, img); + if (img == null) { + return null; + } + BufferedImageCacheEntry cacheEntry = BufferedImageCacheEntry.pngEncoded(img); + Logging.trace("Storing {0} ({1} bytes) in cache...", cacheKey, cacheEntry.getContent().length); + imgCache.put(cacheKey, cacheEntry); } if (!multiResolution || svg == null) @@ -261,6 +307,10 @@ return getImageIcon(iconSize, true, ImageResizeMode.PADDED); } + static String statistics() { + return String.format("ImageResource cache: [%s]", imgCache.getStatistics()); + } + @Override public String toString() { return "ImageResource [" Index: test/unit/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetReaderTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/test/unit/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetReaderTest.java b/test/unit/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetReaderTest.java --- a/test/unit/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetReaderTest.java (revision 18755) +++ b/test/unit/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetReaderTest.java (date 1686747933422) @@ -89,9 +89,10 @@ * @throws IOException if any I/O error occurs */ @Test - void testReadDefaulPresets() throws SAXException, IOException { + void testReadDefaultPresets() throws SAXException, IOException { String presetfile = "resource://data/defaultpresets.xml"; final Collection presets = TaggingPresetReader.readAll(presetfile, true); assertTrue(presets.size() > 0, "Default presets are empty"); + TaggingPresetsTest.waitForIconLoading(presets); } }