Search window speedups, including for very large decks - #11295
Conversation
056a5a7 to
a7105d9
Compare
|
thanks, interesting ideas but rather large: |
Opening a floating zone view with a large deck - most visibly the library, during a tutor search - froze the UI, and sorting, closing or reopening it froze it again each time. This commit covers the structural causes in the refresh path itself; the image work follows separately. - doRefresh matched each card to its existing panel with getCardPanel, a linear scan over the panel list, making a refresh quadratic in zone size. Build a card id to panel index once per refresh and look up in it. Same for the dispose scan in setCardPanels, where List.contains ran once per existing panel. - updatePromptVisibility called getCards() for a single anyMatch, and getCards() is not a getter: it copies, sorts and filters the whole zone every call. Iterate the panels doRefresh just built instead, so a refresh walks the zone once rather than twice. - A single game action can move many cards at once, and each zone-change event rebuilt the whole window. Coalesce bursts of refresh calls into one rebuild. Panel lookups that must see the result immediately - card-move animation and targeting, via the static FloatingZone.getCardPanel - flush the pending refresh first. The coalescing keeps its pending flag in plain state and rebuilds Swing components, so refresh asserts it runs on the EDT, matching the existing assertions in CardPanelContainer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured on a 500-card library with a partially downloaded image
collection, reopening the search window cost ~6.2s, of which ~99% was
inside ImageCache.scaleImage. Two causes, both here.
Placeholder renders were deliberately never cached ("Skip store cache since
the rendering speed seems to be fast enough"). For a card with no local
image that means FCardImageRenderer draws a full 488x680 card face from
scratch, and with ~284 such cards in the deck it ran ~284 times on every
single refresh - about 4.7s of the 6.2s, roughly 16ms per card. Cache them,
track their keys so that image-presence probes aren't fooled into thinking
a real image arrived, and drop them when the fetcher delivers the real one.
The cache was capped at 400 entries while one large zone view needs two per
card - the decoded original and the scaled copy the panel paints - so it
measured ~700 evictions per refresh and could not hold even a single view.
400 was never a considered choice for this workload; it is just what this
preference has always defaulted to. Treat that exact value as unset and use
1500, while leaving any other value alone as one somebody chose. Also
switch to soft values so memory pressure rather than entry count is what
ultimately evicts, which is what this class already documented itself as
doing.
Invalidating a downloaded card's stale variants tracks them in a multimap
rather than scanning the whole cache per download, which would have been
O(cache) per card on the EDT - the cost this change exists to remove.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With placeholder renders now cached, reopening a large zone view is fast, but the first open still decodes every image on the EDT and freezes the UI while it does. Move that work off the EDT. Card panels paint immediately and the image arrives via a callback; JPEG decode, corner rounding and resampling run on ThreadUtil's shared pool, as does ImageKeys.getImageFile with its cascade of File.exists probes. ImageKeys' caches become concurrent collections to suit. Only provably-pure image work leaves the EDT. Key resolution beyond ImageKeys, placeholder rendering (FCardImageRenderer has mutable statics) and Card.getCardForUi were audited as not thread safe and stay on the EDT, with the placeholder path running one card per event so the UI keeps breathing. Asynchronously cached images are recorded in the variant map so a later download still invalidates them. Separately, every layout pass fired componentResized on all panels, each re-running the full setCard. Resizes now refresh only the image, skip requests whose key and size are unchanged, and keep an animation panel's existing image rather than re-requesting one per frame. getImageNoDefault had no remaining callers after this and is removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A card with no local image is shown by drawing its face with FCardImageRenderer, at about 16ms. Only the scaled copy of that render was cached, so any request at a different size redrew it from scratch - and nothing could prime it ahead of time without already knowing what size a view would ask for. The size dependency is not real. The face is always drawn at 488x680 times the screen scale whatever size was requested, and only the resample afterwards is size-specific - roughly a millisecond against sixteen. Cache the render under its own key so any later request at any size skips straight to resampling. The comment here previously declined to cache it, on the grounds that a downloaded image would then be masked by the stale render. Registering the render as a variant of the base key means clearGeneratedVariants drops it when the fetcher delivers the real image, which is the same mechanism the scaled copies already rely on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Even with the cache fixes and asynchronous loading, the first search of a match is always cold: a large library is only ever read when a view first shows it, and for a card with no local image the face has to be drawn on the EDT because FCardImageRenderer shares non-thread-safe statics. On a partially downloaded collection that is the bulk of the first open, and no amount of moving work to background threads addresses it. So do the work before it is needed. At match start, decode the players' deck images on a worker thread. Cards with no local image are handed to the online fetcher so they download during the early game rather than when a search first shows them, and their faces are drawn one per EDT event in the meantime - the same rendering the first search would otherwise do all at once, spread across time nobody is waiting on. Because a drawn face is now cached independently of any display size, this needs no advance knowledge of how large the eventual view will be, and works on a first-ever match. The preload starts after the match views open, because the desktop clears the image cache when switching screens and would otherwise discard it, and cancels whenever the cache is cleared. It is exposed as a default no-op on IGuiBase; only the desktop GUI opts in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9efe953 to
c11667f
Compare
|
Force-pushed a rewrite of this branch — "rather large" was fair, and I wanted to re-review It is now five commits, each one mechanism, each building and passing the desktop test suite Instrumenting it changed the PR rather than just documenting it. I had assumed the cost was That also let me delete things rather than add them. Gone since you last looked:
Net effect is one fewer file touched than the version you commented on, and no new user Two things I would rather flag than have you find:
|
|
good call, that will surely help |
|
Also worth noting Nr. 5 is related to a previous attempt: #7838 |
|
Thanks — that pointer led somewhere I didn't expect. Reading #7838, the concern tehdiplomat raised there turns out not to be specific to that PR. Desktop's on-demand fetcher has never handled rate limiting at all: On the constraint itself, I went looking and found That said, automatic versus user-initiated is your call, not mine — if you'd rather pre-caching stayed something the player asks for, say so and I'll drop commit 5. The measured win in this PR is the rendering work; the fetching commit is the least evidenced part of it. |
What this fixes
With a large (~500 card) deck, any effect that opens the library view froze the UI for
several seconds — and froze it again on right-click sort, on close, and on every reopen.
Measured
Instrumented on a 500-card library, reopening the search window:
99% of the time was inside
ImageCache.scaleImage, and about three quarters of thatwas re-rendering card faces for cards with no local image — ~284 of them, at ~16ms each,
redrawn on every single refresh because placeholder renders were deliberately never cached.
The remainder was image decode, made worse by ~700 cache evictions per refresh against a
400-entry cap that could not hold even one such view.
Panel bookkeeping — the thing that looks quadratic and was — turned out to be ~30ms of the
6156. It is fixed here too, but it was never the problem.
The commits
Zone view refreshes.
doRefreshmatched cards to panels with a linear scan each(quadratic in zone size);
setCardPanelsdid the same in its dispose loop.updatePromptVisibilitycalledgetCards()for oneanyMatch, andgetCards()copies,sorts and filters the whole zone every call — so a refresh walked the zone twice. Bursts
of zone-change events each rebuilt the whole window; those are now coalesced, with a flush
for the animation/targeting lookups that need current panels immediately.
Cache placeholder renders, and size the cache. The 77% above. Placeholder renders are
cached and tracked so image-presence probes aren't fooled, and dropped when the fetcher
delivers the real image. The 400-entry cap is treated as "unset" and raised to 1500 — see
the note below. Soft values so memory pressure, not entry count, is what ultimately evicts.
Load card images asynchronously. Decode, corner rounding, resampling and
ImageKeys.getImageFile'sFile.existscascade move off the EDT. Only provably-pure imagework leaves it: key resolution beyond
ImageKeys, placeholder rendering(
FCardImageRendererhas mutable statics) andCard.getCardForUiwere audited as notthread-safe and stay. Resizes also stop re-running the full
setCardper layout pass.Cache the drawn card face. A face is always drawn at 488x680 times the screen scale
regardless of the size requested, and only the resample afterwards is size-specific —
~16ms against ~1ms. Caching only the scaled copy meant a different size redrew from
scratch. Caching the render itself makes any later request at any size a resample away.
Warm deck card images at match start. The first search of a match was always cold, and
the placeholder rendering that dominates it cannot leave the EDT. So do it early: decode
deck images on a worker, hand missing ones to the online fetcher, and draw their faces one
per EDT event while nobody is waiting. Because (4) caches a face independently of display
size, this needs no advance knowledge of how large the eventual view will be.
Notes for reviewers
On raising the image cache size.
UI_IMAGE_CACHE_MAXIMUMdefaults to 400 and appearsnowhere in the settings UI, so in practice nobody has chosen it — and because
AbstractPreferences.save()writes every key, existing users all have a literal400intheir preferences file, so raising the default alone would fix nothing for them. This treats
exactly 400 as "unset" and uses 1500, leaving any other value alone as one somebody actually
set. Happy to change the approach if you'd rather this were surfaced as a real setting.
Commit 3 is not measurable on the machine this was tested on. With ~57% of that
collection's images missing locally, most cards take the placeholder path, which stays on the
EDT — so moving decode off it changes little there. Its value is arithmetic rather than
observed: ~14ms per card of decode, so on the order of seconds for a user with a complete
local collection. Flagging it rather than letting it ride on the overall result.
The measurements are from one machine and one collection state. The 77% figure is
specific to a partially-downloaded collection. With all images present the split shifts
toward decode and cache sizing.
getImageNoDefaulthad no remaining callers after commit 3 and is removed.Testing
mvn -pl forge-gui-desktop -am test: 286 tests, 0 failures, at every commit in the series.search open/sort/close/reopen, first-search-of-match on a fresh profile, battlefield play,
card-move animations, targeting, stack and game log, and placeholder→downloaded-image
replacement.
🤖 Implemented with the assistance of Claude Code (Opus).