Avoid re-reading the image file on every scaled drawImage - #3506
Open
vogella wants to merge 1 commit into
Open
Conversation
Contributor
This was referenced Aug 13, 2026
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 eclipse-platform#3505
vogella
force-pushed
the
perf/drawimage-isdynamicallysizable
branch
from
August 23, 2026 18:50
61dec63 to
f737a17
Compare
There was a problem hiding this comment.
Pull request overview
This PR reduces repeated disk reads during scaled GC.drawImage(Image, int, int, int, int) when the source Image is backed by a file that cannot be loaded at arbitrary sizes (e.g., PNG/GIF/JPEG). It does this by remembering when a resolved filename has already been proven “non-dynamically-sizable”, and by performing the “format sniff” + “load at size” using a single stream open.
Changes:
- Cache “known non-sizable” resolved filenames in the platform
Imagescaled-draw caches/wrappers to avoid re-sniffing the same static-format file on every repaint. - Add a single-pass loader path (
loadBySizeIfDynamicallySizable) that determines format sizability and loads-at-size from the same opened stream. - Add JUnit coverage asserting that static-format images are not re-read and that sizability is re-evaluated when an
Image’s backing filename changes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java | Adds tests validating the “no re-read for static formats” behavior and re-evaluation when the backing filename changes. |
| bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java | Memoizes the last resolved non-sizable filename in the file-name provider wrapper to skip repeated sniffing/loads for static formats. |
| bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/Image.java | Adds non-sizable filename memoization in CachedImageAtSize to avoid repeated format checks on static files. |
| bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/Image.java | Mirrors GTK’s CachedImageAtSize memoization for static-format files. |
| bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/image/FileFormat.java | Introduces an “if known” format-sizability check that can return “unknown” when the signature can’t be identified. |
| bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageLoader.java | Adds loadBySizeIfDynamicallySizable using mark/reset on a buffered stream to avoid double-opening the file. |
| bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageDataLoader.java | Adds an internal AtSizeLoadResult wrapper and a delegating entry point for the single-pass “check + load” path. |
Suppressed comments (2)
tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java:1266
- This test also depends on device zoom being 100; otherwise the separate zoom-related file decode path can cause file reads that mask the behavior being asserted. Capture/force device zoom to 100 at the start so the test only exercises the scaled-draw sizability caching.
public void test_drawImageAtSize_reevaluatesSizabilityWhenFileNameChanges() throws IOException {
Path sizableFile = tempFolder.resolve("switchable-collapseall.svg");
Files.copy(Path.of(getPath("collapseall.svg")), sizableFile);
AtomicReference<String> currentFile = new AtomicReference<>(getPath("collapseall.png"));
ImageFileNameProvider switchingProvider = zoom -> zoom == 100 ? currentFile.get() : null;
tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java:1287
- If the test forces the device zoom, it should be restored in the finally block. Restore it before deleting the temp file so an IOException from delete doesn't leave the global zoom modified for subsequent tests.
gc.dispose();
target.dispose();
image.dispose();
Files.deleteIfExists(sizableFile);
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1234
to
+1255
| 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); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The scaled
GC.drawImageoverload askedisDynamicallySizable(fileName)on every draw and then discarded the answer, so for any PNG, GIF or JPEG the backing file was opened and sniffed again for each repaint, forever.Since sizability is a property of the file and never depends on the requested size, this remembers the file already found not to be sizable, keyed on the resolved file name so that an
Imagebacked by different files over its lifetime still behaves correctly.A new
ImageLoader.loadBySizeIfDynamicallySizabledoes the format check and the load from a single open, which also closes the window where the file could change between the two reads.Measured with
strace -f -e trace=openatover 100 draws of oneImageon Linux/GTK at device zoom 100, opens per draw drop from 1.00 to 0.01 for PNG (stable and alternating draw sizes) and from 2.00 to 1.00 for SVG at alternating sizes.That is roughly 1.5 to 2.5 us per draw on a 16x16 icon, and the rendered output is bit-identical before and after.
Andrey's concern on the issue about NFS-mounted config areas applies directly here, since this was one
openatper scaled draw on that path.Two limits of that table are worth stating so nobody reads it as covering HiDPI.
At device zoom 200 the same PNG case measures 2.00 opens per draw before and 1.00 after, rather than 0.01.
The open that remains is a different defect, #3507: the internal
drawImagereads the source dimensions fromgetImageData(), which re-decodes the file at any zoom other than 100.That one is fixed separately in #3510, and with both applied the case reaches 0.00.
This PR also does not fix
CachedImageAtSize.isReusable, which compares a pixel value against a point value:refreshconverts the requested size withDPIUtil.pointToPixel(destWidth, getDeviceZoom()), while the cached image'swidthis divided back down to points inImage.init(ImageData, zoom).At zoom 200 that comparison can never be true, so the single-entry size cache never hits there and every draw is a miss.
The change here makes each of those misses cheap rather than making them stop happening; the unit mismatch deserves its own fix.
One behavior change worth noting: a file replaced in place at the same path is no longer re-sniffed for the lifetime of the
Image.Fixes #3505