From f737a1711f7b305c7adebebc348aaeffac5db666 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Thu, 13 Aug 2026 18:51:10 +0200 Subject: [PATCH] Avoid re-reading the image file on every scaled drawImage CachedImageAtSize.loadImageDataAtExactSize asked ImageDataLoader.isDynamicallySizable(fileName) whether the backing file can be loaded at an arbitrary size, then discarded the answer. For a format that cannot (PNG, GIF, JPEG, so nearly every icon) the cached image stayed null, so the open plus format sniff repeated on every draw. For SVG each cache miss opened the file twice, to sniff and to load. Sizability depends on the file alone and never on the requested size, so remember the file already found not to be sizable. The memo is keyed on the resolved file name, because an Image may be backed by different files. The new ImageLoader.loadBySizeIfDynamicallySizable does the check and the load from a single open. Opens per draw over 100 draws of one Image, strace on Linux/GTK: PNG, stable draw size 1.00 -> 0.01 PNG, alternating draw size 1.00 -> 0.01 SVG, stable draw size 0.02 -> 0.01 SVG, alternating draw size 2.00 -> 1.00 That is 1.5 to 2.5 us per draw on a 16x16 icon, and rendering is bit-identical. One behavior change: a file replaced in place at the same path is no longer re-sniffed for the lifetime of the Image. Fixes https://github.com/eclipse-platform/eclipse.platform.swt/issues/3505 --- .../cocoa/org/eclipse/swt/graphics/Image.java | 15 ++++- .../eclipse/swt/graphics/ImageDataLoader.java | 21 ++++--- .../org/eclipse/swt/graphics/ImageLoader.java | 37 ++++++----- .../swt/internal/image/FileFormat.java | 11 +++- .../gtk/org/eclipse/swt/graphics/Image.java | 15 ++++- .../win32/org/eclipse/swt/graphics/Image.java | 18 ++++-- .../Test_org_eclipse_swt_graphics_Image.java | 63 +++++++++++++++++++ 7 files changed, 145 insertions(+), 35 deletions(-) diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/Image.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/Image.java index dbf1b852f5c..b7d01d158cc 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/Image.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/Image.java @@ -1909,6 +1909,8 @@ void executeOnImageAtSizeBestFittingSize(Consumer imageAtBestFittingSizeC private class CachedImageAtSize { private Image image; + /** File already found not to be dynamically sizable, so re-reading it cannot help. */ + private String nonSizableFileName; public void destroy() { if (image != null) { @@ -1959,10 +1961,17 @@ private Optional loadImageDataAtExactSize(int targetWidth, int target } if (imageFileNameProvider != null) { String fileName = DPIUtil.validateAndGetImagePathAtZoom(imageFileNameProvider, 100).element(); - if (ImageDataLoader.isDynamicallySizable(fileName)) { - ImageData imageDataAtSize = ImageDataLoader.loadBySize(fileName, targetWidth, targetHeight); - return Optional.of(imageDataAtSize); + if (fileName.equals(nonSizableFileName)) { + return Optional.empty(); } + ImageDataLoader.AtSizeLoadResult imageDataAtSize = ImageDataLoader + .loadBySizeIfDynamicallySizable(fileName, targetWidth, targetHeight); + if (imageDataAtSize.staticFormat()) { + nonSizableFileName = fileName; + } else if (imageDataAtSize.data().isPresent()) { + nonSizableFileName = null; + } + return imageDataAtSize.data(); } return Optional.empty(); } diff --git a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageDataLoader.java b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageDataLoader.java index 5f3bbc118f8..5656ea1ca86 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageDataLoader.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageDataLoader.java @@ -45,10 +45,6 @@ public static boolean canLoadAtZoom(String filename, int fileZoom, int targetZoo return ImageLoader.canLoadAtZoom(filename, fileZoom, targetZoom); } - static boolean isDynamicallySizable(String filename) { - return ImageLoader.isDynamicallySizable(filename); - } - static boolean isDynamicallySizable(InputStream stream) { return ImageLoader.isDynamicallySizable(stream); } @@ -71,10 +67,19 @@ public static ImageData loadBySize(InputStream stream, int width, int height) { return data; } - public static ImageData loadBySize(String filename, int width, int height) { - ImageData data = new ImageLoader().loadBySize(filename, width, height); - if (data == null) SWT.error(SWT.ERROR_INVALID_IMAGE); - return data; + public static AtSizeLoadResult loadBySizeIfDynamicallySizable(String filename, int width, int height) { + return new ImageLoader().loadBySizeIfDynamicallySizable(filename, width, height); + } + + /** + * Outcome of an at-size load attempt. {@code staticFormat} is only set when the file was + * positively identified as a format that cannot be loaded at an arbitrary size, so that a + * format which merely could not be determined is not remembered as non-sizable. + */ + record AtSizeLoadResult(Optional data, boolean staticFormat) { + + static final AtSizeLoadResult STATIC_FORMAT = new AtSizeLoadResult(Optional.empty(), true); + static final AtSizeLoadResult UNKNOWN_FORMAT = new AtSizeLoadResult(Optional.empty(), false); } } diff --git a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageLoader.java b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageLoader.java index 66e8e294f44..289e35499a2 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageLoader.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageLoader.java @@ -55,6 +55,9 @@ */ public class ImageLoader { + /** Upper bound on the bytes a format signature check may consume before the stream is rewound. */ + private static final int SIGNATURE_LOOKAHEAD_LIMIT = 8192; + /** * the array of ImageData objects in this ImageLoader. * This array is read in when the load method is called, @@ -209,16 +212,6 @@ List> loadByZoom(String filename, int fileZoom, int tar return null; } -ImageData loadBySize(String filename, int width, int height) { - if (filename == null) SWT.error(SWT.ERROR_NULL_ARGUMENT); - try (InputStream stream = new FileInputStream(filename)) { - return loadBySize(stream, width, height); - } catch (IOException e) { - SWT.error(SWT.ERROR_IO, e); - } - return null; -} - static boolean canLoadAtZoom(String filename, int fileZoom, int targetZoom) { if (filename == null) SWT.error(SWT.ERROR_NULL_ARGUMENT); try (InputStream stream = new FileInputStream(filename)) { @@ -229,13 +222,29 @@ static boolean canLoadAtZoom(String filename, int fileZoom, int targetZoom) { return false; } -static boolean isDynamicallySizable(String filename) { - try (InputStream stream = new FileInputStream(filename)) { - return FileFormat.isDynamicallySizableFormat(stream); +/** + * Loads the image at the given size if the file is a dynamically sizable format, + * reading the file only once for both the format check and the load. + */ +ImageDataLoader.AtSizeLoadResult loadBySizeIfDynamicallySizable(String filename, int width, int height) { + if (filename == null) SWT.error(SWT.ERROR_NULL_ARGUMENT); + try (InputStream stream = new BufferedInputStream(new FileInputStream(filename))) { + stream.mark(SIGNATURE_LOOKAHEAD_LIMIT); + Optional sizable = FileFormat.isDynamicallySizableFormatIfKnown(stream); + if (sizable.isEmpty()) { + return ImageDataLoader.AtSizeLoadResult.UNKNOWN_FORMAT; + } + if (!sizable.get()) { + return ImageDataLoader.AtSizeLoadResult.STATIC_FORMAT; + } + stream.reset(); + ImageData data = loadBySize(stream, width, height); + if (data == null) SWT.error(SWT.ERROR_INVALID_IMAGE); + return new ImageDataLoader.AtSizeLoadResult(Optional.of(data), false); } catch (IOException e) { SWT.error(SWT.ERROR_IO, e); } - return false; + return ImageDataLoader.AtSizeLoadResult.UNKNOWN_FORMAT; } static boolean isDynamicallySizable(InputStream stream) { diff --git a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/image/FileFormat.java b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/image/FileFormat.java index dfd5e265199..a0f89f18074 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/image/FileFormat.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/image/FileFormat.java @@ -73,8 +73,15 @@ private static Optional determineFileFormat(LEDataInputStream stream private static final int MAX_SIGNATURE_BYTES = 18 + 2; // e.g. Win-BMP or OS2-BMP plus a safety-margin public static boolean isDynamicallySizableFormat(InputStream is) { - Optional format = determineFileFormat(new LEDataInputStream(is, MAX_SIGNATURE_BYTES)); - return format.isPresent() && !(format.get() instanceof StaticImageFileFormat); + return isDynamicallySizableFormatIfKnown(is).orElse(Boolean.FALSE); + } + + /** + * @return empty if no format could be identified, so sizability is unknown + */ + public static Optional isDynamicallySizableFormatIfKnown(InputStream is) { + return determineFileFormat(new LEDataInputStream(is, MAX_SIGNATURE_BYTES)) + .map(format -> !(format instanceof StaticImageFileFormat)); } static abstract class StaticImageFileFormat extends FileFormat { diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/Image.java b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/Image.java index 4c8593108c6..030d83db103 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/Image.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/Image.java @@ -970,6 +970,8 @@ void destroy() { private class CachedImageAtSize { private Image image; + /** File already found not to be dynamically sizable, so re-reading it cannot help. */ + private String nonSizableFileName; public void destroy() { if (image != null) { @@ -1020,10 +1022,17 @@ private Optional loadImageDataAtExactSize(int targetWidth, int target } if (imageFileNameProvider != null) { String fileName = DPIUtil.validateAndGetImagePathAtZoom(imageFileNameProvider, 100).element(); - if (ImageDataLoader.isDynamicallySizable(fileName)) { - ImageData imageDataAtSize = ImageDataLoader.loadBySize(fileName, targetWidth, targetHeight); - return Optional.of(imageDataAtSize); + if (fileName.equals(nonSizableFileName)) { + return Optional.empty(); } + ImageDataLoader.AtSizeLoadResult imageDataAtSize = ImageDataLoader + .loadBySizeIfDynamicallySizable(fileName, targetWidth, targetHeight); + if (imageDataAtSize.staticFormat()) { + nonSizableFileName = fileName; + } else if (imageDataAtSize.data().isPresent()) { + nonSizableFileName = null; + } + return imageDataAtSize.data(); } return Optional.empty(); } diff --git a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java b/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java index 098efadcb9d..f14904a12ff 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java @@ -2619,6 +2619,9 @@ protected Rectangle getBounds(int zoom) { } private class ImageFileNameProviderWrapper extends BaseImageProviderWrapper { + /** File already found not to be dynamically sizable, so re-reading it cannot help. */ + private String nonSizableFileName; + ImageFileNameProviderWrapper(ImageFileNameProvider provider) { super(provider, ImageFileNameProvider.class); // Checks for the contract of the passed provider require @@ -2871,12 +2874,17 @@ private long extractHandleForPixelFormat(int width, int height, int pixelFormat) @Override protected Optional loadImageDataAtExactSize(int targetWidth, int targetHeight) { String fileName = DPIUtil.validateAndGetImagePathAtZoom(this.provider, 100).element(); - if (ImageDataLoader.isDynamicallySizable(fileName)) { - ImageData imageDataAtSize = ImageDataLoader.loadBySize(fileName, targetWidth, targetHeight); - ImageData adaptedImageDataAtSize = adaptImageDataIfDisabledOrGray(imageDataAtSize); - return Optional.of(adaptedImageDataAtSize); + if (fileName.equals(nonSizableFileName)) { + return Optional.empty(); } - return Optional.empty(); + ImageDataLoader.AtSizeLoadResult imageDataAtSize = ImageDataLoader + .loadBySizeIfDynamicallySizable(fileName, targetWidth, targetHeight); + if (imageDataAtSize.staticFormat()) { + nonSizableFileName = fileName; + } else if (imageDataAtSize.data().isPresent()) { + nonSizableFileName = null; + } + return imageDataAtSize.data().map(Image.this::adaptImageDataIfDisabledOrGray); } } diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java index 020dd94af01..a53c943ee5c 100644 --- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java +++ b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java @@ -34,6 +34,7 @@ import java.nio.file.Path; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import org.eclipse.swt.SWT; @@ -1224,5 +1225,67 @@ public void test_gcOnImageGcDrawer_imageDataAtNonDeviceZoom() { } } +/** + * Whether a file can be loaded at an arbitrary size never changes, so the scaled + * drawImage must determine it once instead of re-reading the file on every draw. + * Deleting the file after the first draw makes any further read fail loudly. + */ +@Test +public void test_drawImageAtSize_doesNotReReadFileOfNonSizableFormat() throws IOException { + Path file = tempFolder.resolve("volatile-collapseall.png"); + Files.copy(Path.of(getPath("collapseall.png")), file); + + Image image = new Image(display, file.toString()); + Image target = new Image(display, 64, 64); + GC gc = new GC(target); + try { + Rectangle bounds = image.getBounds(); + gc.drawImage(image, 0, 0, bounds.width * 2, bounds.height * 2); + + Files.delete(file); + + gc.drawImage(image, 0, 0, bounds.width * 2, bounds.height * 2); + gc.drawImage(image, 0, 0, bounds.width * 3, bounds.height * 3); + } finally { + gc.dispose(); + target.dispose(); + image.dispose(); + Files.deleteIfExists(file); + } +} + +/** + * An Image may be backed by different files over its lifetime, so what is known about + * one file must not be applied to the next one. + */ +@Test +public void test_drawImageAtSize_reevaluatesSizabilityWhenFileNameChanges() throws IOException { + Path sizableFile = tempFolder.resolve("switchable-collapseall.svg"); + Files.copy(Path.of(getPath("collapseall.svg")), sizableFile); + AtomicReference currentFile = new AtomicReference<>(getPath("collapseall.png")); + ImageFileNameProvider switchingProvider = zoom -> zoom == 100 ? currentFile.get() : null; + + Image image = new Image(display, switchingProvider); + Image target = new Image(display, 64, 64); + GC gc = new GC(target); + try { + // learns that the PNG cannot be loaded at an arbitrary size + gc.drawImage(image, 0, 0, 20, 20); + + currentFile.set(sizableFile.toString()); + gc.drawImage(image, 0, 0, 20, 20); + + // if the SVG were still treated as non-sizable, it would never be read at all + Files.delete(sizableFile); + assertThrows(SWTException.class, () -> gc.drawImage(image, 0, 0, 24, 24), + "the file of a sizable image must be read again for a new size"); + } finally { + gc.dispose(); + target.dispose(); + image.dispose(); + Files.deleteIfExists(sizableFile); + } +} + }