gui: the batch screen stops slowing down - a control is wired once, and the minimal set remembers each format's smallest size - #128
Merged
Conversation
…atest address, and the minimal set remembers the smallest size of each format The batch screen registers every field again on every rebuild and keeps the controls. Fields.listen and Fields.counter wrapped the control's callback on every registration, so after k rebuilds one change was reported k times, under every address the control had ever had, and a size box counted into every caption it had been drawn with. The handler is now put in once (wiredOnce, kept on the control) and pointed at the latest address and caption on every registration. With a preset switched on, every change expanded empty-and-minimal, which works out the smallest size of each format by planning it - encoding the pictures. preset.smallest remembers it per format id: 50.4 MB allocated per expansion before, 0.51 MB after. Measured in the real window, interleaved, two runs each: the preset switch 357-369 ms -> 25-49 ms, a key typed with a preset on 1.7-1.9 s -> ~0 ms, the process after typing 509 MB -> 363-382 MB. Guards: a control registered five times reports once under its last address and counts into its last caption, no control stands under two addresses on any screen, and a second expansion of the minimal set stays under 5 MB. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tes, so listen nests four deep Co-Authored-By: Claude Opus 5.5 <[email protected]>
… the band Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (14)
🧰 Additional context used📓 Path-based instructions (10)Applies to text shown to the user (labels, buttons, tooltips, placeholders, dialogs, errors, status messages, empty states, translations).⚙️ CodeRabbit configuration file Files:
Verify tests check real behavior and would fail if the implementation were broken.⚙️ CodeRabbit configuration file Files:
Performance is a known weak spot of these projects.⚙️ CodeRabbit configuration file Files:
Applies only to code that builds or styles a GUI.⚙️ CodeRabbit configuration file Files:
User-facing changelog.⚙️ CodeRabbit configuration file Files:
SECURITY, HIGH PRIORITY.⚙️ CodeRabbit configuration file Files:
Go code.⚙️ CodeRabbit configuration file Files:
Check that documentation matches the actual code in this PR: commands, flags, config keys, file paths, build steps and examples must exist.⚙️ CodeRabbit configuration file Files:
All code in this repository is written by an AI coding agent (Claude Code).⚙️ CodeRabbit configuration file Files:
Source excerpt: **Words a user reads are English, with a flat hyphen and no semicolons.**📄 CodeRabbit inference engine (CONTRIBUTING.md) Files:
🔇 Additional comments (6)
📝 WalkthroughWalkthroughThe change installs GUI reporting and byte-count callbacks once per control while updating their targets on registration. It also caches each format’s smallest accepted size by format ID. Tests cover reporting targets, control registries, and allocations across repeated preset expansions. ChangesGUI and preset updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Merge Risk: ⚪ Minimal · up to Batch-screen controls now report each change once under their current setting, and preset activation computes each format's smallest size once. No outstanding defects were found, and the change appears ready to merge. 🚥 Pre-merge checks | ✅ 14✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
…, where concurrency is already declared CI on #128 was red on every system with one guard: TestConcurrencyStaysWhereItWasPutOnPurpose - the preset package had grown a sync.Map of its own. The memory moves to format.SmallestWithLabel in registry.go, which is already declared as the place the registry's reads meet its writes, and sits beside Register, whose refusal of a second descriptor under one id is what makes the id a safe key. The size is worked out without the lock held, because planning an archive reads the registry. Co-Authored-By: Claude Opus 5.5 <[email protected]>
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
1 participant
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.
What was wrong
Measured in the real window (Windows, hardware OpenGL) with a probe that drives it through the toolkit's own queue and times a heartbeat on its one goroutine:
Start from a presetswitch onSeveral batchesgot slower with every press: 0.7 s at the first, 6.1 s at the twentieth.Two causes:
Fields.listenandFields.counterwrapped a control's change callback on every registration. The batch screen registers every field again on every rebuild (an address carries the batch's position) and keeps the controls (so typed text survives). After k rebuilds one change was reported k times, under every address the control had ever had, and a size box counted into every caption it had ever been drawn with.empty-and-minimal, which works out the smallest size of each format by planning it - encoding the pictures. 50.4 MB allocated per expansion.What changed
wiredOnce/chainOnce(internal/gui/parts/fields.go): the handler is put into a control once and re-pointed at the latest address and caption on every registration. The state lives on the control (Entry,Chooser,Toggle), not in a map onFields, which would keep a removed batch's boxes alive.preset.smallestremembers each format's smallest size per process, by id (format.Registerrefuses a second descriptor under one id). 0.51 MB per expansion.Measured after (interleaved with
main, two runs each)Guards
TestAControlRegisteredAgainReportsOnceUnderItsLatestAddress- a box, a menu and a switch registered five times report once, under the last address, and count into the last caption (red before the fix).TestNoControlIsRegisteredUnderTwoAddressesAtOnce- the assumption the fix rests on, asked of all three work screens, the batch screen with three batches, archive contents and a preset.TestTheMinimalSetIsWorkedOutOnceAndNotAtEveryExpansion- a second expansion stays under 5 MB.crowdedDepthFunctionsratcheted 50 -> 49.Not in this PR: most of the window's remaining memory is fonts parsed once per theme-override scope (26 sets of ~7.4 MB). That is the next, separate change.
🤖 Generated with Claude Code
Summary by CodeRabbit