From 2b913791c43a89d07ff4e8a76d42b64e10e1fc85 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Thu, 24 Sep 2026 10:15:10 +0200 Subject: [PATCH 1/2] gui: the window gives memory back once it has been left alone After a spell of work the process kept about 200 MB for as long as the window stood idle. Fyne destroys the renderers of objects that left the screen only while drawing a frame, and an idle window draws none. Go also returns freed pages slowly and collects in the quiet only every two minutes. tidy.go waits out one quiet for the whole window. Every reading of the form restarts it (watchedSettle, formerly countedSettle, tells runner.touched). After 70 s - past the minute Fyne keeps a renderer - the canvas root is marked for redrawing, which touches no widget: the scroll, the focus and every box, menu and switch were compared either side and were the same. 12 s later - past the ten seconds between two of Fyne's cleans - memory is given back with go debug.FreeOSMemory(). That runs off the window's thread because one of ten calls on it took 2491 ms. Nothing is given back while a run owns the window, and the wait then starts again. In the real window, with no nudge from the probe: 215 MB -> 129 MB between 80 and 90 s of quiet. The concurrency scan did not see time.AfterFunc, so the window's clock in internal/gui/run_cgo.go had been calling back on a timer's goroutine undeclared. The scan now sees it and is asked of source written in the test. A declared file with no concurrency left is red. run_cgo.go and tidy.go are declared and on the race job's watched list. Guards: TestTheWindowGivesMemoryBackOnceItHasBeenLeftAlone, TestSomethingDoneDuringTheWaitStartsTheQuietOver, TestNoMemoryIsGivenBackWhileWorkIsGoing, on a quiet clock in the guards' host kept apart from the busy face's, and TestTheConcurrencyScanSeesEachKindItLooksFor. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 2 +- CHANGELOG.md | 7 ++ internal/guard/concurrency_test.go | 163 ++++++++++++++++++++++------- internal/guard/settleonce_test.go | 2 +- internal/guard/tidy_test.go | 118 +++++++++++++++++++++ internal/guard/window_test.go | 61 +++++++++++ internal/gui/window/generate.go | 2 +- internal/gui/window/open.go | 46 ++++---- internal/gui/window/preset.go | 2 +- internal/gui/window/recipe.go | 2 +- internal/gui/window/run.go | 6 ++ internal/gui/window/tidy.go | 137 ++++++++++++++++++++++++ 12 files changed, 490 insertions(+), 58 deletions(-) create mode 100644 internal/guard/tidy_test.go create mode 100644 internal/gui/window/tidy.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 10e991b..51b2cc2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -693,7 +693,7 @@ jobs: # in somebody else's file. run: | set -euo pipefail - watched='internal/format/registry.go internal/damage/damage.go cmd/tfg/main.go internal/gui/window/run.go internal/audit/parallel.go internal/engine/parallel.go go.mod' + watched='internal/format/registry.go internal/damage/damage.go cmd/tfg/main.go internal/gui/window/run.go internal/gui/run_cgo.go internal/gui/window/tidy.go internal/audit/parallel.go internal/engine/parallel.go go.mod' # On a pull request there is no "before" - the field belongs to a push # - so this asked for something empty and every pull request answered # "touched". That quietly undid the decision of 2026-08-20, because diff --git a/CHANGELOG.md b/CHANGELOG.md index ddb5031..38dd78a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -494,6 +494,13 @@ because it turns other people's test suites red. again for each one. Both are now done once, and the window uses less memory while you type. +- **The window gives memory back once it has been left alone.** After a + spell of work it used to keep about 200 MB for as long as it stood idle. + About a minute and a half after the last thing done in it, the window now + gives back what it no longer uses - 215 MB down to 129 MB in one + measurement. Nothing on the screen moves when it does. A minimised window + gives back less, because the toolkit only lets go while it draws. + - **Changing a setting of a preset is about four times faster.** With `upload-validation`, changing one of its settings held the window for about 0.15 seconds, and so did every key typed on `Several batches` built diff --git a/internal/guard/concurrency_test.go b/internal/guard/concurrency_test.go index cbbda54..f673c8b 100644 --- a/internal/guard/concurrency_test.go +++ b/internal/guard/concurrency_test.go @@ -44,6 +44,18 @@ var mayBeConcurrent = map[string]string{ // invariant G7 exists to hold. Added 2026-08-05 with the first generate // window, and the owner was told. "internal/gui/window/run.go": "the run happens beside the window, and closing the window waits for it", + // The window's clock. time.AfterFunc calls back on a goroutine of its + // own, which is how a delayed piece of work - the busy face, the wait for + // quiet - waits without holding the window, and the callback hands its + // work straight to the toolkit's thread. Declared 2026-09-24, when the scan + // learned to see a timer's callback: it had been here since the busy face + // got its delay, unlisted. + "internal/gui/run_cgo.go": "the window's clock calls back on a timer's goroutine and hands the work to the toolkit's thread", + // Giving memory back beside the window rather than on its thread: one of + // ten FreeOSMemory calls measured on the window's thread took 2491 ms. + // The goroutine touches nothing of ours. Added 2026-09-24, and THE OWNER + // DECIDED IT - docs/GUI-MEMORY-2026-09-23.md section 4j. + "internal/gui/window/tidy.go": "memory is given back beside the window, so the window never waits on it", // Hashing the files a manifest claims is the work verify and cleanup are // made of, and it is embarrassingly parallel. Added 2026-09-05 and the // owner decided it: O116 turned the same idea down on 2026-08-20 on a @@ -95,7 +107,7 @@ func isCancellation(n ast.Node) bool { } func TestConcurrencyStaysWhereItWasPutOnPurpose(t *testing.T) { - var found []string + var found, idle []string for _, p := range packages(t) { for _, path := range p.files { @@ -104,50 +116,39 @@ func TestConcurrencyStaysWhereItWasPutOnPurpose(t *testing.T) { rel = path } rel = filepath.ToSlash(rel) - if _, allowed := mayBeConcurrent[rel]; allowed { - continue - } fset := token.NewFileSet() file, err := parser.ParseFile(fset, path, nil, 0) if err != nil { t.Fatalf("parsing %s: %v", path, err) } - - ast.Inspect(file, func(n ast.Node) bool { - if n == nil { - return false - } - what := "" - switch node := n.(type) { - case *ast.GoStmt: - what = "starts a goroutine" - case *ast.ChanType: - what = "declares a channel" - case *ast.SendStmt: - what = "sends on a channel" - case *ast.SelectStmt: - // A select waiting only on cancellation is how every long - // loop here notices Ctrl+C. - if onlyCancellation(node) { - return true - } - what = "selects over channels" - case *ast.SelectorExpr: - if id, ok := node.X.(*ast.Ident); ok && (id.Name == "sync" || id.Name == "atomic") { - what = "uses " + id.Name + "." + node.Sel.Name - } - } - if what == "" { - return true + here := concurrencyIn(fset, file) + if _, allowed := mayBeConcurrent[rel]; allowed { + if len(here) == 0 { + idle = append(idle, rel) } - found = append(found, fmt.Sprintf("%s:%d %s", - rel, fset.Position(n.Pos()).Line, what)) - return true - }) + continue + } + for _, one := range here { + found = append(found, rel+":"+one) + } } } + // The other direction, since 2026-09-24. A file declared here that runs + // nothing beside anything is a declaration nobody can check - and it is + // how a decision gets undone quietly: the window gives memory back on a + // goroutine BECAUSE a call on its own thread took 2491 ms once, and taking + // the go statement away would leave the file declared and every guard + // green. A file the build leaves out on this machine is not asked. + if len(idle) > 0 { + sort.Strings(idle) + t.Errorf("declared as concurrent and running nothing beside anything:\n %s\n\n"+ + "Either the concurrency moved and the declaration should go with it, or it was taken out and\n"+ + "the reason it was put there - written beside the declaration - no longer holds.", + strings.Join(idle, "\n ")) + } + if len(found) > 0 { sort.Strings(found) t.Errorf("concurrency turned up in %d place(s) outside the files that declare it:\n %s\n\n"+ @@ -157,6 +158,98 @@ func TestConcurrencyStaysWhereItWasPutOnPurpose(t *testing.T) { } } +// concurrencyIn is every place in one file that runs something beside the +// code around it, as "line what". +// +// A timer's callback since 2026-09-24. time.AfterFunc runs the function it is +// handed on a goroutine of its own, and this scan saw only the go statement - +// so the window's clock, which has handed every delayed piece of work to such +// a goroutine since the busy face got a delay, stood outside the list without +// a word. Found while the window was taught to give memory back +// (docs/GUI-MEMORY-2026-09-23.md section 4j), decided by the owner that day. +func concurrencyIn(fset *token.FileSet, file *ast.File) []string { + var found []string + ast.Inspect(file, func(n ast.Node) bool { + if n == nil { + return false + } + what := "" + switch node := n.(type) { + case *ast.GoStmt: + what = "starts a goroutine" + case *ast.ChanType: + what = "declares a channel" + case *ast.SendStmt: + what = "sends on a channel" + case *ast.SelectStmt: + // A select waiting only on cancellation is how every long + // loop here notices Ctrl+C. + if onlyCancellation(node) { + return true + } + what = "selects over channels" + case *ast.SelectorExpr: + what = concurrentName(node) + } + if what == "" { + return true + } + found = append(found, fmt.Sprintf("%d %s", fset.Position(n.Pos()).Line, what)) + return true + }) + return found +} + +// concurrentName is what a package-qualified name says about running beside +// something, or nothing: a lock or an atomic, or a timer that calls back on +// a goroutine of its own. +func concurrentName(node *ast.SelectorExpr) string { + id, ok := node.X.(*ast.Ident) + if !ok { + return "" + } + switch { + case id.Name == "sync" || id.Name == "atomic": + return "uses " + id.Name + "." + node.Sel.Name + case id.Name == "time" && node.Sel.Name == "AfterFunc": + return "hands a callback to a timer's goroutine (time.AfterFunc)" + } + return "" +} + +// The scan sees each kind of concurrency it names, asked of source written +// here - because the tree may hold none of a kind outside the declared files, +// and a scan that stopped seeing it would then stay green. +func TestTheConcurrencyScanSeesEachKindItLooksFor(t *testing.T) { + for _, c := range []struct{ name, body string }{ + {"a goroutine", `go f()`}, + {"a channel", `var c chan int; _ = c`}, + {"a lock", `var m sync.Mutex; m.Lock()`}, + {"a timer's callback", `time.AfterFunc(0, f)`}, + } { + src := "package x\nfunc f() {}\nfunc g() {\n" + c.body + "\n}\n" + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "x.go", src, 0) + if err != nil { + t.Fatalf("%s: %v", c.name, err) + } + if len(concurrencyIn(fset, file)) == 0 { + t.Errorf("the scan does not see %s: %s", c.name, c.body) + } + } + // And a name from package time that runs nothing beside anything is not + // reported, so the timer rule is about the callback and not the package. + src := "package x\nfunc g() {\n_ = time.Now()\n}\n" + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "x.go", src, 0) + if err != nil { + t.Fatal(err) + } + if got := concurrencyIn(fset, file); len(got) != 0 { + t.Errorf("the scan calls a plain duration concurrency: %v", got) + } +} + // onlyCancellation reports whether every case of a select is either a receive // from something named Done or the default branch. func onlyCancellation(s *ast.SelectStmt) bool { diff --git a/internal/guard/settleonce_test.go b/internal/guard/settleonce_test.go index c12a3d3..ec51b36 100644 --- a/internal/guard/settleonce_test.go +++ b/internal/guard/settleonce_test.go @@ -56,7 +56,7 @@ func TestOneChangeOfABoxReadsTheFormOnce(t *testing.T) { switch { case host.settles == 0: t.Fatalf("typing %q into %q on the %s screen told the host of no reading of the form - "+ - "either the box reports no change or the screen reads its form without countedSettle, and this guard sees nothing either way", + "either the box reports no change or the screen reads its form without watchedSettle, and this guard sees nothing either way", v, b.at, b.screen) case host.settles > 1: t.Errorf("typing %q into %q on the %s screen read the form %d times, expected once - "+ diff --git a/internal/guard/tidy_test.go b/internal/guard/tidy_test.go new file mode 100644 index 0000000..fd05447 --- /dev/null +++ b/internal/guard/tidy_test.go @@ -0,0 +1,118 @@ +package guard + +import ( + "testing" + "time" + + "github.com/donislawdev/TestingFilesGenerator/internal/gui/text" + "github.com/donislawdev/TestingFilesGenerator/internal/gui/window" +) + +// The window gives memory back once it has been left alone, and only then. +// +// After a spell of work the process kept 189-206 MB for as long as the window +// stood idle: the toolkit lets go of the renderers of what left the screen only +// while drawing a frame, and an idle window draws none. The window now waits +// out a quiet, asks for one frame, and gives the memory back a little later - +// 252-263 MB to 117-119 MB in the real window (docs/GUI-MEMORY-2026-09-23.md +// section 4j). +// +// What this can hold is the timing and the order. That the memory really comes +// back is a property of the toolkit and of Go, and only the real window shows +// it - tools/probes/guilag -quiet -nudge. +// +// The waits are asked about as well as the order. The first has to outlast the +// minute the toolkit keeps a renderer, or the frame comes while they are still +// valid and nothing is let go. The second has to outlast the ten seconds between +// two of the toolkit's cleans, or the memory goes back before the renderers do. +func TestTheWindowGivesMemoryBackOnceItHasBeenLeftAlone(t *testing.T) { + host := newFakeHost(t) + window.Open(host) + host.quiet = &quietClock{} + content := tabNamed(t, host.content, text.TabOneTarget()) + + fill(t, content, text.FieldSeed(), "5") + quiet := host.quiet.waiting() + if len(quiet) != 1 { + t.Fatalf("typing into a box left %d wait(s) for quiet, expected one - the window cannot tell when it has been left alone", len(quiet)) + } + if quiet[0].after <= time.Minute { + t.Errorf("the window waits %v of quiet, and the toolkit keeps a renderer for a minute - the frame would come while they are still valid", quiet[0].after) + } + + host.quiet.fireAll() + if host.releases != 0 { + t.Fatal("memory was given back at the end of the quiet, before the frame that lets the renderers go") + } + frame := host.quiet.waiting() + if len(frame) != 1 { + t.Fatalf("after the quiet the window waits for %d thing(s), expected the one release", len(frame)) + } + if frame[0].after <= 10*time.Second { + t.Errorf("memory goes back %v after the frame, and the toolkit cleans at most once in ten seconds - it would sometimes go back before the renderers", frame[0].after) + } + + host.quiet.fireAll() + if host.releases != 1 { + t.Fatalf("the quiet and the frame were waited out and memory was given back %d time(s), expected once", host.releases) + } + host.quiet.fireAll() + if host.releases != 1 || len(host.quiet.waiting()) != 0 { + t.Errorf("with nothing done since, memory was given back %d time(s) and %d wait(s) are left - it has to happen once per quiet, not keep going", + host.releases, len(host.quiet.waiting())) + } +} + +// Anything done during the wait starts the quiet over, and takes back a +// release already waiting. +func TestSomethingDoneDuringTheWaitStartsTheQuietOver(t *testing.T) { + host := newFakeHost(t) + window.Open(host) + host.quiet = &quietClock{} + content := tabNamed(t, host.content, text.TabOneTarget()) + + fill(t, content, text.FieldSeed(), "5") + host.quiet.fireAll() // the quiet waited out, so the release is waiting + if len(host.quiet.waiting()) != 1 { + t.Fatal("after the quiet no release is waiting, so this guard is not in the state it asks about") + } + + fill(t, content, text.FieldSeed(), "6") + host.quiet.fireAll() // the new quiet, which has to come first + if host.releases != 0 { + t.Error("something was typed while the release was waiting and memory was given back anyway, in the middle of somebody working") + } + host.quiet.fireAll() + if host.releases != 1 { + t.Errorf("after the new quiet and its frame memory was given back %d time(s), expected once", host.releases) + } +} + +// No memory goes back while work owns the window - the run is what is using it. +func TestNoMemoryIsGivenBackWhileWorkIsGoing(t *testing.T) { + host, content, hold := heldScreen(t) + host.quiet = &quietClock{} + fill(t, content, text.FieldOutputDir(), t.TempDir()) + press(t, content, text.ButtonGenerate()) + + hold.look(func() { + if len(host.quiet.waiting()) == 0 { + t.Fatal("the run started and no wait for quiet was asked for, so this guard asks about nothing") + } + host.quiet.fireAll() // the quiet ends during the run + host.quiet.fireAll() + if host.releases != 0 { + t.Error("memory was given back while a run owned the window") + } + if len(host.quiet.waiting()) == 0 { + t.Error("the quiet ended during a run and nothing waits for the next one, so memory is never given back after it") + } + }) + join(host) + + host.quiet.fireAll() + host.quiet.fireAll() + if host.releases != 1 { + t.Errorf("the run ended, the quiet was waited out, and memory was given back %d time(s), expected once", host.releases) + } +} diff --git a/internal/guard/window_test.go b/internal/guard/window_test.go index e673b11..b483f11 100644 --- a/internal/guard/window_test.go +++ b/internal/guard/window_test.go @@ -98,8 +98,69 @@ type fakeHost struct { // settles and expansions count what Settling and ExpandingPreset were told. settles int expansions int + + // quiet is the clock the window waits out its quiet on, when a guard holds + // it - see QuietLater. Nil means the wait never ends, and releases counts + // the times the window gave memory back. + quiet *quietClock + releases int +} + +// quietClock keeps every request for later, each with whether it was called +// off - a list rather than one slot, because what the guard asks is whether a +// request already waiting was taken back when something happened. +type quietClock struct { + pending []*quietRequest + asked int +} + +type quietRequest struct { + after time.Duration + then func() + calledOff bool +} + +// fireAll runs every request waiting that was not called off, in the order +// they were asked for, and forgets them. What they ask for while running +// waits for the next call. +func (c *quietClock) fireAll() { + due := c.pending + c.pending = nil + for _, r := range due { + if !r.calledOff { + r.then() + } + } } +// waiting is the requests not called off. +func (c *quietClock) waiting() []*quietRequest { + var out []*quietRequest + for _, r := range c.pending { + if !r.calledOff { + out = append(out, r) + } + } + return out +} + +// QuietLater is the window's clock for waiting out its quiet (tidy.go), kept +// apart from Later so that a request asked for on every key cannot take the +// place of the busy face a guard is holding. Never fires unless a guard holds +// it - no guard here lasts the seventy seconds a real window waits. +func (h *fakeHost) QuietLater(after time.Duration, then func()) func() { + if h.quiet == nil { + return func() {} + } + r := &quietRequest{after: after, then: then} + h.quiet.pending = append(h.quiet.pending, r) + h.quiet.asked++ + return func() { r.calledOff = true } +} + +// ReleasingMemory counts the window giving memory back. +func (h *fakeHost) ReleasingMemory() { h.releases++ } + // heldClock keeps what the window asked for later, so a guard can look at the // screen BEFORE the busy face arrives and then let it arrive. type heldClock struct { diff --git a/internal/gui/window/generate.go b/internal/gui/window/generate.go index 2d22ce2..b0e90bb 100644 --- a/internal/gui/window/generate.go +++ b/internal/gui/window/generate.go @@ -202,7 +202,7 @@ type Generate struct { // NewGenerate builds the screen. links are the buttons to the other screens. func NewGenerate(host Host, links ...fyne.CanvasObject) *Generate { g := &Generate{runner: newRunner(host.Later), host: host, tips: parts.NewTips(), settingsFolded: true} - g.runner.settle = countedSettle(host, g.settle) + g.runner.settle = watchedSettle(host, g.runner, g.settle) g.runner.offer.through(host) // This screen is one target and draws its boxes under the bare key, so a // refusal that arrives carrying a position belongs to the box of that name. diff --git a/internal/gui/window/open.go b/internal/gui/window/open.go index b6d0ec0..5b7131c 100644 --- a/internal/gui/window/open.go +++ b/internal/gui/window/open.go @@ -114,7 +114,10 @@ func Open(h Host) fyne.Size { // on the signal handler in cmd/tfg, and closing a window is not a signal - // so without this the run would carry on with nobody watching it, or die in // the middle of a file. - closeCleanly(h, []interface{ Stop() }{gen, pre, rec}, working, &showing) + // One wait for quiet for the whole window, told by every screen, and + // stopped with them when the window closes - see tidy.go. + quiet := tidyWhenLeftAlone(h, gen.runner, pre.runner, rec.runner) + closeCleanly(h, []interface{ Stop() }{gen, pre, rec, quiet}, working, &showing) offerSettling(h, []interface{ Settled() }{gen, pre, rec}) offerHolding(h, []interface{ HoldBeforeFinishing(func()) }{gen, pre, rec}) @@ -450,30 +453,37 @@ func offerHolding(h Host, screens []interface{ HoldBeforeFinishing(func()) }) { } } -// countedSettle is a screen's reading of its form, told to a host that counts -// the readings and handed back untouched to any other. -// -// The same shape as the two above - an optional interface, checked rather than -// required, and nothing in the shipped program implements it. It is here for a -// number a guard cannot read off the screen: how many times one change of a -// box read the form. It was two until 2026-09-23, once for the line and once -// for the box, and on the preset screen every reading expanded the preset - -// 271-295 ms of the window's thread per key for upload-validation, measured -// in docs/GUI-MEMORY-2026-09-23.md section 4h. -func countedSettle(h Host, settle settler) settler { - c, ok := h.(interface{ Settling() }) - if !ok { - return settle - } +// watchedSettle is a screen's reading of its form, with two things told about +// it on the way. +// +// The window's wait for quiet is told, so that memory is given back only once +// the window has been left alone - see tidy.go. Every change somebody makes and +// every run reads the form, so this is the one place that hears all of them. +// +// A host that counts the readings is told as well. The same shape as the two +// above - an optional interface, checked rather than required, and nothing in +// the shipped program implements it. It is here for a number a guard cannot +// read off the screen: how many times one change of a box read the form. It +// was two until 2026-09-23, once for the line and once for the box, and on the +// preset screen every reading expanded the preset - 271-295 ms of the +// window's thread per key for upload-validation, measured in +// docs/GUI-MEMORY-2026-09-23.md section 4h. +func watchedSettle(h Host, r *runner, settle settler) settler { + c, counts := h.(interface{ Settling() }) return func() ([]engine.Target, engine.Options, error) { - c.Settling() + if counts { + c.Settling() + } + if r.touched != nil { + r.touched() + } return settle() } } // tellExpanding says to a host that counts them that a preset is being // expanded rather than taken from what the screen remembers - see -// lastExpansion. The same kind of seam as countedSettle, for the other half +// lastExpansion. The same kind of seam as watchedSettle, for the other half // of the same question. func tellExpanding(h Host) { if c, ok := h.(interface{ ExpandingPreset() }); ok { diff --git a/internal/gui/window/preset.go b/internal/gui/window/preset.go index d1395f2..aa0dc62 100644 --- a/internal/gui/window/preset.go +++ b/internal/gui/window/preset.go @@ -60,7 +60,7 @@ type Preset struct { func NewPreset(host Host, links ...fyne.CanvasObject) *Preset { p := &Preset{runner: newRunner(host.Later), host: host, tips: parts.NewTips()} p.runner.offer.through(host) - p.runner.settle = countedSettle(host, p.settle) + p.runner.settle = watchedSettle(host, p.runner, p.settle) // No readdress here, and that is the boundary of this screen rather than an // omission. The other two screens draw boxes for the settings of a target, // so a refusal carrying a position has a box to be moved onto. This one diff --git a/internal/gui/window/recipe.go b/internal/gui/window/recipe.go index 4d4c013..b81970b 100644 --- a/internal/gui/window/recipe.go +++ b/internal/gui/window/recipe.go @@ -162,7 +162,7 @@ type content struct { // also what the single batch screen shows, so the two read as one tool. func NewRecipe(host Host, links ...fyne.CanvasObject) *Recipe { r := &Recipe{runner: newRunner(host.Later), host: host, tips: parts.NewTips()} - r.runner.settle = countedSettle(host, r.settle) + r.runner.settle = watchedSettle(host, r.runner, r.settle) r.runner.offer.through(host) // A refusal about a size belongs on the box the switch is showing. r.runner.readdress = r.readdressSizeWay diff --git a/internal/gui/window/run.go b/internal/gui/window/run.go index 8a5e24a..a353c59 100644 --- a/internal/gui/window/run.go +++ b/internal/gui/window/run.go @@ -199,6 +199,12 @@ type runner struct { // the real one, so the run says which number it made up. The command line // prints these as "note:" lines and this is the window's half of it. notes []string + + // touched is told at every reading of the form - see watchedSettle - so + // that the window gives memory back once it has been left alone. Every + // change somebody makes and every run reads the form. Nil on a screen + // built on its own. See tidy.go. + touched func() } // Fields is every box on this screen, for a guard to compare against the tree. diff --git a/internal/gui/window/tidy.go b/internal/gui/window/tidy.go new file mode 100644 index 0000000..445f766 --- /dev/null +++ b/internal/gui/window/tidy.go @@ -0,0 +1,137 @@ +package window + +import ( + "runtime/debug" + "time" +) + +// Giving memory back to the system once the window has been left alone. +// +// After a spell of work - batches added and taken away, a base preset switched, +// a run - the process kept 189-206 MB for as long as the window stood idle, +// measured over five minutes on 2026-09-23 (docs/GUI-MEMORY-2026-09-23.md +// section 4j). Two things hold it, and neither is ours to change: +// +// - The toolkit keeps the renderers of objects that have left the screen for +// a minute, and destroys them only while drawing a frame (Fyne 2.8.1, +// internal/cache/base.go Clean). A window nobody touches draws no frames, +// so they stay. +// - Go gives the pages a collection freed back to the system slowly, and runs +// a collection in the quiet only every two minutes. +// +// So after tidyAfterQuiet of nothing, one frame is asked for - the root of the +// canvas marked for redrawing, which touches no widget and moves nothing: the +// scroll, the keyboard, what every box, menu and switch holds were compared +// either side and were the same. tidyAfterFrame later the memory is given +// back. Measured: 252-263 MB to 117-119 MB, in four runs of four. +// +// Given back beside the window rather than on its thread, and that is the +// owner's decision of 2026-09-24: one of ten calls of FreeOSMemory measured on +// the window's thread took 2491 ms against 4.6-16.2 ms for the rest, for a +// reason nobody found, and it would land on somebody coming back to the +// window. So this file starts a goroutine, and it is declared as concurrent +// for that reason. The goroutine touches nothing of ours. +// +// What it does not do: a minimised window draws no frames either, so there the +// renderers stay and only what Go holds goes back. + +// tidyAfterQuiet is how long nothing has to happen. Past the minute the +// toolkit keeps a renderer for (Fyne cache.ValidDuration), so the ones that +// left the screen with the last change have expired by then. +const tidyAfterQuiet = 70 * time.Second + +// tidyAfterFrame is how long after the frame the memory goes back. The toolkit +// cleans at most once in ten seconds and a clean that comes sooner waits for +// the next, so two seconds - what the first measurement used - would sometimes +// give back before the renderers were gone. +const tidyAfterFrame = 12 * time.Second + +// tidyWhenLeftAlone gives the window its one wait for quiet and has every +// screen tell it at every reading of the form - which every change and every +// run makes. +func tidyWhenLeftAlone(h Host, screens ...*runner) *tidy { + t := newTidy(h, func() bool { return anyBusy(screens) }) + for _, r := range screens { + r.touched = t.touch + } + return t +} + +// anyBusy says whether work owns any of these screens. +func anyBusy(screens []*runner) bool { + for _, r := range screens { + if r.busy.occupied { + return true + } + } + return false +} + +// tidy is the one quiet period being waited out, for the whole window. +type tidy struct { + host Host + // busy says whether any screen has work going. Memory is not given back + // under a run - the run is what is using it - and the wait starts again, + // so a run longer than the wait is followed by a release once it ends. + busy func() bool + // later is the host's clock, or a clock of its own a guard hands in - see + // newTidy. + later later + callOff func() +} + +// newTidy builds the wait for one window. +// +// On the host's clock unless the host has a separate one for this. A guard's +// host holds ONE pending request - the busy face's, which several guards hold +// and fire - and a quiet period asked for on every key would take its place. +// So the guards' host keeps these apart, and the program's host, whose clock +// is a timer per request, needs nothing of the kind. +func newTidy(h Host, busy func() bool) *tidy { + t := &tidy{host: h, busy: busy, later: h.Later} + if q, ok := h.(interface { + QuietLater(after time.Duration, then func()) func() + }); ok { + t.later = q.QuietLater + } + return t +} + +// touch is told that something happened, and starts the quiet over. +func (t *tidy) touch() { + t.Stop() + t.callOff = t.later(tidyAfterQuiet, t.frame) +} + +// Stop calls off whatever is being waited for. Closing the window stops it, +// along with every screen. +func (t *tidy) Stop() { + if t.callOff != nil { + t.callOff() + t.callOff = nil + } +} + +// frame asks the toolkit to draw, so that it lets go of what has expired. Even +// under a run, which costs one frame and nothing else - whether to give memory +// back is asked once, in release. +func (t *tidy) frame() { + t.callOff = nil + if c := t.host.Canvas(); c != nil && c.Content() != nil { + c.Refresh(c.Content()) + } + t.callOff = t.later(tidyAfterFrame, t.release) +} + +// release gives the memory back, beside the window. +func (t *tidy) release() { + t.callOff = nil + if t.busy() { + t.touch() + return + } + if r, ok := t.host.(interface{ ReleasingMemory() }); ok { + r.ReleasingMemory() + } + go debug.FreeOSMemory() +} From 786dba71be5fac1e4f12bbf217f31c470fb41788 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Thu, 24 Sep 2026 10:34:50 +0200 Subject: [PATCH 2/2] gui: a wait for quiet called off on its way does nothing when it arrives The window's clock hands what it fires to the toolkit's queue (desktop.Later, time.AfterFunc then fyne.Do), and calling it off stops the timer, not a call already queued. If somebody typed in that gap, the quiet they broke still ran: it gave memory back twelve seconds later, in the middle of their work, and took the place of the new quiet's handle, so closing the window could no longer call that one off. tidy now counts the waits called off and each call checks it is still its own - the same as busy.epoch. The concurrency guard reported a declared file that runs nothing, but only among the files the walk reaches. A declared file that is gone kept its declaration and its place on the race job's list, green. The check is a function asked of a folder made in the test. The changelog said memory goes back after the last thing done in the window. It is the last setting changed or press of Preview or Generate - scrolling does not count - and it waits while work runs. Guards: TestAWaitCalledOffOnItsWayDoesNothingWhenItArrives (red on the old tidy.go, 2 waits and 2 releases), TestADeclarationWhoseFileIsGoneIsReported. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 10 ++++--- internal/guard/concurrency_test.go | 35 +++++++++++++++++++++++- internal/guard/tidy_test.go | 43 ++++++++++++++++++++++++++++++ internal/gui/window/tidy.go | 30 ++++++++++++++++----- 4 files changed, 107 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78d65ca..8038b04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -496,10 +496,12 @@ because it turns other people's test suites red. - **The window gives memory back once it has been left alone.** After a spell of work it used to keep about 200 MB for as long as it stood idle. - About a minute and a half after the last thing done in it, the window now - gives back what it no longer uses - 215 MB down to 129 MB in one - measurement. Nothing on the screen moves when it does. A minimised window - gives back less, because the toolkit only lets go while it draws. + About a minute and a half after the last setting changed or the last press + of Preview or Generate, the window now gives back what it no longer uses - + 215 MB down to 129 MB in one measurement. Scrolling does not count, and + while Preview or Generate is running it waits. Nothing on the screen moves + when it does. A minimised window gives back less, because the toolkit only + lets go while it draws. - **Changing a setting of a preset is about four times faster.** With `upload-validation`, changing one of its settings held the window for diff --git a/internal/guard/concurrency_test.go b/internal/guard/concurrency_test.go index f673c8b..8c847ae 100644 --- a/internal/guard/concurrency_test.go +++ b/internal/guard/concurrency_test.go @@ -140,7 +140,11 @@ func TestConcurrencyStaysWhereItWasPutOnPurpose(t *testing.T) { // how a decision gets undone quietly: the window gives memory back on a // goroutine BECAUSE a call on its own thread took 2491 ms once, and taking // the go statement away would leave the file declared and every guard - // green. A file the build leaves out on this machine is not asked. + // green. A file the build leaves out on this machine is not asked. A file + // that is gone is, because the walk above never reaches it and a deleted + // file would otherwise keep its declaration and its place on the race + // detector's list - an outside review of the pull request named it. + idle = append(idle, declaredWithoutAFile(repoRoot(t), mayBeConcurrent)...) if len(idle) > 0 { sort.Strings(idle) t.Errorf("declared as concurrent and running nothing beside anything:\n %s\n\n"+ @@ -158,6 +162,35 @@ func TestConcurrencyStaysWhereItWasPutOnPurpose(t *testing.T) { } } +// declaredWithoutAFile is every declared path with no file under root, each +// with what the system said about it. +func declaredWithoutAFile(root string, declared map[string]string) []string { + var gone []string + for rel := range declared { + if _, err := os.Stat(filepath.Join(root, filepath.FromSlash(rel))); err != nil { + gone = append(gone, rel+" ("+err.Error()+")") + } + } + return gone +} + +// A declaration whose file is gone is reported, and one whose file is there +// is not - asked of a folder made here, because every file the tree declares +// exists and a check that stopped looking would stay green on it. +func TestADeclarationWhoseFileIsGoneIsReported(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "a"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "a", "here.go"), []byte("package a\n"), 0o644); err != nil { + t.Fatal(err) + } + got := declaredWithoutAFile(root, map[string]string{"a/here.go": "", "a/gone.go": ""}) + if len(got) != 1 || !strings.HasPrefix(got[0], "a/gone.go ") { + t.Errorf("declared a/here.go, which exists, and a/gone.go, which does not - reported %q, expected a/gone.go alone", got) + } +} + // concurrencyIn is every place in one file that runs something beside the // code around it, as "line what". // diff --git a/internal/guard/tidy_test.go b/internal/guard/tidy_test.go index fd05447..7e14cbe 100644 --- a/internal/guard/tidy_test.go +++ b/internal/guard/tidy_test.go @@ -88,6 +88,49 @@ func TestSomethingDoneDuringTheWaitStartsTheQuietOver(t *testing.T) { } } +// A wait called off on its way does nothing when it arrives. +// +// Calling the clock off is not enough, and an outside review of the pull +// request named why: the real window's clock hands what it fires to the +// toolkit's queue (desktop.Later, time.AfterFunc then fyne.Do), and a call +// already queued still runs. If somebody types in that gap, the quiet they +// broke arrived anyway - it gave memory back twelve seconds later, in the +// middle of their work, and took the place of the new quiet's handle, so +// closing the window could no longer call that one off. The same gap as the +// busy face's (TestAFaceAskedForByEarlierWorkNeverDressesLaterWork). +// +// Played out with the held clock: the first quiet is kept aside, a second key +// calls it off, and then it is fired as if the queue had just got round to it. +func TestAWaitCalledOffOnItsWayDoesNothingWhenItArrives(t *testing.T) { + host := newFakeHost(t) + window.Open(host) + host.quiet = &quietClock{} + content := tabNamed(t, host.content, text.TabOneTarget()) + + fill(t, content, text.FieldSeed(), "5") + first := host.quiet.waiting() + if len(first) != 1 { + t.Fatalf("typing into a box left %d wait(s) for quiet, expected one to keep aside", len(first)) + } + fill(t, content, text.FieldSeed(), "6") + if !first[0].calledOff { + t.Fatal("a second key did not call the first quiet off, so there is no call on its way to ask about") + } + + first[0].then() // the first quiet arriving from the queue after all + if got := len(host.quiet.waiting()); got != 1 { + t.Errorf("a quiet called off on its way arrived and %d wait(s) are left, expected the one quiet of the second key", got) + } + host.quiet.fireAll() // the second key's quiet + if host.releases != 0 { + t.Error("memory was given back after a quiet somebody had broken, in the middle of their work") + } + host.quiet.fireAll() + if host.releases != 1 { + t.Errorf("after the second key's quiet and its frame memory was given back %d time(s), expected once", host.releases) + } +} + // No memory goes back while work owns the window - the run is what is using it. func TestNoMemoryIsGivenBackWhileWorkIsGoing(t *testing.T) { host, content, hold := heldScreen(t) diff --git a/internal/gui/window/tidy.go b/internal/gui/window/tidy.go index 445f766..0a6ed10 100644 --- a/internal/gui/window/tidy.go +++ b/internal/gui/window/tidy.go @@ -78,6 +78,12 @@ type tidy struct { // newTidy. later later callOff func() + // wait counts the waits called off, so that one called off on its way + // does nothing when it arrives. Calling the clock off is not enough: the + // real window's clock hands what it fires to the toolkit's queue, and a + // call already queued when somebody types still runs. The same as + // busy.epoch, and an outside review of the pull request named it. + wait int } // newTidy builds the wait for one window. @@ -100,32 +106,44 @@ func newTidy(h Host, busy func() bool) *tidy { // touch is told that something happened, and starts the quiet over. func (t *tidy) touch() { t.Stop() - t.callOff = t.later(tidyAfterQuiet, t.frame) + t.after(tidyAfterQuiet, t.frame) } -// Stop calls off whatever is being waited for. Closing the window stops it, -// along with every screen. +// Stop calls off whatever is being waited for, a call already on its way +// included. Closing the window stops it, along with every screen. func (t *tidy) Stop() { + t.wait++ if t.callOff != nil { t.callOff() t.callOff = nil } } +// after asks the clock for then, which arrives only if nothing called the +// wait off in the meantime. +func (t *tidy) after(d time.Duration, then func()) { + mine := t.wait + t.callOff = t.later(d, func() { + if t.wait != mine { + return + } + t.callOff = nil + then() + }) +} + // frame asks the toolkit to draw, so that it lets go of what has expired. Even // under a run, which costs one frame and nothing else - whether to give memory // back is asked once, in release. func (t *tidy) frame() { - t.callOff = nil if c := t.host.Canvas(); c != nil && c.Content() != nil { c.Refresh(c.Content()) } - t.callOff = t.later(tidyAfterFrame, t.release) + t.after(tidyAfterFrame, t.release) } // release gives the memory back, beside the window. func (t *tidy) release() { - t.callOff = nil if t.busy() { t.touch() return