Fix per-file icon caching for extensions with custom icons (.exe) - #3762
Conversation
|
The reason the existing Win32UIEnhancer::getFileIcon has the "Must be SWT Thread" requirement is that the method "getFileIconSupport" performs (internal) SWT operations via reflection - e.g. Image::win32_new. As (generally speaking) public SWT operations insist on being called on the SWT thread (and are therefore inherently single-threaded) it was felt wise to ensure these interactions were also performed on the SWT thread. While you have obviously done some testing, potential edge cases caused by multi-threaded access to code that assumes it is single threaded can be hard to trigger and when they do can call all sorts of weird failure modes... |
|
Also I'd question memory usage. There are users with 1000s of downloads with 1000s of files in each, so potentially millions of full path strings (there is an existing mechanism to reduce this) linked to millions of (probably mainly identical) images |
The noCacheExtList mechanism was dead code. The check that would have routed .exe files to a per-file cache key sat behind noAWT == false in getPathIcon(), but forceNoAWT is always true on Windows and macOS, so execution always reached getIconFromExtension() and cached every icon under the shared extension key "osicon.exe". The first .exe icon rendered was then reused for every other .exe file. ImageRepository.java: - move the noCacheExtList check into getIconFromExtension() where it is effective; matching files resolve to a per-file entry rather than the shared extension key. the existing ignore_icon_exts list is honoured first, so a user who has excluded an extension still gets no shell lookup for it - the slow part (checking the file is reachable) runs on a background AsyncDispatcher; the icon lookup itself goes through the existing getFileIcon() on the SWT thread, so its "must be SWT thread" contract and its timeout / circuit-breaker protection both still hold. the background dispatcher blocks until each lookup completes, which throttles them one at a time rather than flooding the SWT queue - cache the resulting Image by icon content (SHA-1 of the image data plus geometry) rather than per path, so the common case of many files sharing an installer icon collapses onto a single Image - bound the path -> content map at 512 entries (LRU), so a library with thousands of downloads cannot accumulate an unbounded number of full path strings - record an empty result for a failed or unreachable file so it isn't retried on every repaint; the entry ages out of the map normally, letting a file that has become readable retry - add an optional listener to getPathIcon, run once an asynchronously resolved icon has landed, so a caller that has already painted can refresh itself - use fileExistsWithTimeout rather than File.exists(), as the latter can block on a dead network share TorrentUIUtilsV3.java: - the thumbnail cache stores whatever getPathIcon returned, so a placeholder returned before the per-file icon resolved would stick there for the rest of the session and no repaint would replace it. use the new listener to refresh the cached thumbnail and fire contentImageLoaded(image, false), which is what ColumnThumbAndName already keys off to invalidate the cell NameItem.java (mytorrents): - pass the full path via getFile(true).getPath(), matching what files/NameItem, files/PathNameItem and ColumnThumbAndName already do, so per-file entries are unique per file rather than colliding on the display name - invalidate the cell when a late icon arrives, for the same reason
b1e1b25 to
3f46688
Compare
|
Thanks — both points taken, and the threading one especially. You're right that the background call broke the single-threaded assumption. I'd read the gfi_dispatcher + sem.reserve(2500) structure in getFileIcon as "the support call runs off-thread anyway", but the SWT thread is blocked for the duration there, so nothing else touches SWT while it runs. My version had it running alongside a live SWT thread, which is not the same thing at all. Reworked so the lookup goes through the existing getFileIcon() on the SWT thread via execSWTThread, keeping its contract and its timeout/breaker. Only the reachability check stays on the background dispatcher, and it blocks until each lookup returns so they're throttled one at a time rather than flooding the SWT queue. getFileIconDirect is gone and Win32UIEnhancer is untouched now. I also missed that the per-file path bypassed ignore_icon_exts — it honours that list first now. On memory: the Image is keyed by icon content (SHA-1 of the image data plus geometry) rather than by path, so the many-files-sharing-one-installer-icon case collapses onto a single Image. The path -> content map is an LRU bounded at 512 entries, so the number of full path strings is capped regardless of library size, and a failed or unreachable file records an empty marker so it isn't re-probed on every repaint. I put it through a FUSE filesystem modelling a 4200rpm 2.5" SMR USB drive (~36ms per exists()) to check those hold:
What that setup can't cover is the shell call itself, since Constants.isWindows is false there — the real SHGetFileInfo behaviour, its timeout and the breaker only get exercised on Windows, where I've been running it against ~1200 .exe torrents. One more thing came out of testing it on Windows: the thumbnail cache in TorrentUIUtilsV3 stores whatever getPathIcon hands back, so the placeholder returned before the per-file icon resolved stayed there for the rest of the session - getContentImage kept serving it without ever reaching getPathIcon again, so no amount of scrolling or repainting replaced it. The new listener refreshes the cached thumbnail and fires contentImageLoaded(image, false), which is what ColumnThumbAndName already keys off to invalidate the cell. Diff is 3 files, +363/-10, with no changes to the platform code. |
All .exe files show the same icon on Windows and macOS.
The noCacheExtList mechanism in getPathIcon() that would route them to a per-file cache key is dead code: the check sits below
if (noAWT) return getIconFromExtension(...), and noAWT isforceNoAWT || !bBigwhere forceNoAWT isConstants.isOSX || Constants.isWindows— always true on those platforms. Execution returns before reaching noCacheExtList, so every .exe is cached under the shared key "osicon.exe" and the first icon rendered is reused for all the others.Changes:
Win32UIEnhancer is unchanged. Tested on Windows with ~1300 .exe torrents.