-
-
Notifications
You must be signed in to change notification settings - Fork 2
preset: a set asks each format for its smallest size once, and plans one file once #132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1aecaad
d07059f
18e0b5f
0815238
96cc12b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,29 +23,60 @@ import ( | |||||||||||||||||||||||||||||||||||||
| // was taken by hand on 2026-09-08 and again either side of that move on | ||||||||||||||||||||||||||||||||||||||
| // 2026-09-22, both times 1298 B and this sum. This is that measurement kept. | ||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||
| // Every preset since 2026-09-24, and a refusal's words as well as a source's | ||||||||||||||||||||||||||||||||||||||
| // bytes. Until then only size-boundaries was pinned, and the change that day | ||||||||||||||||||||||||||||||||||||||
| // was to how the other four work their sets out: asking the format for its | ||||||||||||||||||||||||||||||||||||||
| // smallest size once rather than for every file, and planning a file the set | ||||||||||||||||||||||||||||||||||||||
| // holds twelve times once (docs/GUI-MEMORY-2026-09-23.md section 4j). Neither | ||||||||||||||||||||||||||||||||||||||
| // may move a byte, so the gate came first and was measured on the tree before | ||||||||||||||||||||||||||||||||||||||
| // the change. The cases are the ones that reach what changed - other formats | ||||||||||||||||||||||||||||||||||||||
| // in allow, which ask for other floors, a limit small enough that a refusal | ||||||||||||||||||||||||||||||||||||||
| // names the floor, and a spread narrow enough to reach it. | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+26
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win This comment says the change skipped The comment says the change that day affected only "the other four" presets. This PR also changes
A later reader could decide that the pinned Proposed fix-// Every preset since 2026-09-24, and a refusal's words as well as a source's
-// bytes. Until then only size-boundaries was pinned, and the change that day
-// was to how the other four work their sets out: asking the format for its
-// smallest size once rather than for every file, and planning a file the set
-// holds twelve times once (docs/GUI-MEMORY-2026-09-23.md section 4j). Neither
+// Every preset since 2026-09-24, and a refusal's words as well as a source's
+// bytes. Until then only size-boundaries was pinned, and the change that day
+// was to how all five work their sets out: asking the format for its
+// smallest size once rather than for every file, and planning a file the set
+// holds twelve times once (docs/GUI-MEMORY-2026-09-23.md section 4j). NeitherAs per path instructions: "comments explain WHY, not WHAT. Flag comments that no longer match the code." 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||
| // What to do when it goes red: decide, rather than update. The sum moving is a | ||||||||||||||||||||||||||||||||||||||
| // breaking change under D11 - a major, a Breaking entry in the changelog, and | ||||||||||||||||||||||||||||||||||||||
| // the owner's decision, because untouchable rule 12 says the assistant does not | ||||||||||||||||||||||||||||||||||||||
| // raise the version. A refactor that moved it is a refactor to undo. | ||||||||||||||||||||||||||||||||||||||
| func TestEjectingAPresetGivesTheBytesItAlwaysGave(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||
| // bytes and sum are the whole document, refused the whole refusal. The | ||||||||||||||||||||||||||||||||||||||
| // first row was measured 2026-09-08, the rest on 2026-09-24 before the | ||||||||||||||||||||||||||||||||||||||
| // change they guard. | ||||||||||||||||||||||||||||||||||||||
| pinned := []struct { | ||||||||||||||||||||||||||||||||||||||
| id string | ||||||||||||||||||||||||||||||||||||||
| args preset.Args | ||||||||||||||||||||||||||||||||||||||
| // bytes and sum are the whole document, measured 2026-09-08 and | ||||||||||||||||||||||||||||||||||||||
| // unchanged since. | ||||||||||||||||||||||||||||||||||||||
| bytes int | ||||||||||||||||||||||||||||||||||||||
| sum string | ||||||||||||||||||||||||||||||||||||||
| id string | ||||||||||||||||||||||||||||||||||||||
| args preset.Args | ||||||||||||||||||||||||||||||||||||||
| bytes int | ||||||||||||||||||||||||||||||||||||||
| sum string | ||||||||||||||||||||||||||||||||||||||
| refused string | ||||||||||||||||||||||||||||||||||||||
| }{ | ||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||
| id: "size-boundaries", | ||||||||||||||||||||||||||||||||||||||
| args: preset.Args{"limit": "10mb", "format": "pdf"}, | ||||||||||||||||||||||||||||||||||||||
| bytes: 1298, | ||||||||||||||||||||||||||||||||||||||
| sum: "2733cf63db40465fb97e26790d668d65ea01f5e94927a44ddf0869399beee2bb", | ||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||
| {id: "size-boundaries", args: preset.Args{"format": "pdf", "limit": "10mb"}, bytes: 1298, sum: "2733cf63db40465fb97e26790d668d65ea01f5e94927a44ddf0869399beee2bb"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "size-boundaries", args: preset.Args{"format": "png", "limit": "1mb"}, refused: "the preset size-boundaries cannot build this set - under_1mb would be 0 B, and a file cannot be smaller than nothing. Raise the limit above 1048650 B, narrow the spread, or choose a format with a smaller minimum. The limit asked for was 1048576 B."}, | ||||||||||||||||||||||||||||||||||||||
| {id: "size-boundaries", args: preset.Args{"format": "jpg", "limit": "10mb"}, bytes: 1298, sum: "29c7e0a133fb97fdf9d19fb40d0d96ad97c4a1fef67556a9dc734b520f7d9b09"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "size-boundaries", args: preset.Args{"format": "jpg", "limit": "2kb", "spread": "1kb"}, bytes: 640, sum: "1346f4a7ae514fe2d15de426b910e308442f66fa97b485932d8fa1f5df56b910"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "size-boundaries", args: preset.Args{"format": "jpg", "limit": "300", "spread": "100"}, refused: "the preset size-boundaries cannot build this set - under_100 would be 200 B and the smallest JPG this build makes is 602 B. Raise the limit above 702 B, narrow the spread, or choose a format with a smaller minimum. The limit asked for was 300 B."}, | ||||||||||||||||||||||||||||||||||||||
| {id: "upload-validation", args: preset.Args{}, bytes: 4116, sum: "a75039d859ee25d5ea5fd463ac2a774aacb1c45d013cf7ef3b80d46e0688fb78"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "upload-validation", args: preset.Args{"limit": "5mb"}, bytes: 4108, sum: "4b7e716e2e199837b3c2bef228921e0087e894c49a8cf868a246c0ac636b9a35"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "upload-validation", args: preset.Args{"limit": "3kb"}, refused: "the preset upload-validation cannot build this set - allowed_pdf would be 1536 B and the smallest PDF this build makes is 3415 B. Raise the limit to 6830 B or more, or take pdf out of the allowed types. The limit asked for was 3072 B."}, | ||||||||||||||||||||||||||||||||||||||
| {id: "upload-validation", args: preset.Args{"allow": "docx,gif"}, bytes: 3816, sum: "33975195adf794dd9f6e96dbec8a9775bfbda3c4b7dfd3524475f70c39496059"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "upload-validation", args: preset.Args{"allow": "xlsx,ico,wav"}, bytes: 4125, sum: "c46aba3ad1aa6bf3ac2442bde058b9802b4d4f72375fd2f7cbffc6fab1620d6f"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "upload-validation", args: preset.Args{"bulk": "3", "far-over": "off"}, bytes: 3933, sum: "98bc51d5edb94a80fb764a03915eccc6997b70fa8eac153578320b3128cb6d95"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "upload-validation", args: preset.Args{"deny": "exe,js"}, bytes: 3767, sum: "55d9946e7233761716849f52517cd58f7a38c8748f4cdae8620afade0681621c"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "tabular-import", args: preset.Args{}, bytes: 3369, sum: "fad20b41756327a6e85da93715fb09b20db394fb23e4b3ce22bdaf6e11b20726"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "tabular-import", args: preset.Args{"rows": "100"}, bytes: 3367, sum: "ee28ae5421d4717fb24ee6dfbef53f7a54e3a00105e5015f884d59768181d552"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "tabular-import", args: preset.Args{"columns": "5"}, bytes: 3367, sum: "580d526546720b851ac7d834b97c163f5ff50b29caeff5b27fcc1ba00e5bb952"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "text-encoding", args: preset.Args{}, bytes: 4570, sum: "de27b9dc6c646baebaa0b16019ba3ce15d0f1d145941d376b47263de242998e6"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "text-encoding", args: preset.Args{"sample": "8kb"}, bytes: 4570, sum: "f87c73864e5f517abb08b50393cd9a1681a90a30560c1d14dfddcf31e8037479"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "empty-and-minimal", args: preset.Args{}, bytes: 3811, sum: "80641962ac9dfb303f812fd78e0a0d1080f714094d159d7b10448291debb9279"}, | ||||||||||||||||||||||||||||||||||||||
| {id: "empty-and-minimal", args: preset.Args{"formats": "jpg,png,txt"}, bytes: 743, sum: "4fd23e4b06a2e27ede987ab48a2cc302cc2accb9f948c7d5d25f675681f31f69"}, | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| for _, want := range pinned { | ||||||||||||||||||||||||||||||||||||||
| expanded, err := preset.Expand(want.id, want.args) | ||||||||||||||||||||||||||||||||||||||
| if want.refused != "" { | ||||||||||||||||||||||||||||||||||||||
| if err == nil || err.Error() != want.refused { | ||||||||||||||||||||||||||||||||||||||
| t.Errorf("%s at %v was refused with\n %q\nuntil now, and now gives\n %v", want.id, want.args, want.refused, err) | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||
| t.Errorf("%s refused %v: %v", want.id, want.args, err) | ||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -55,8 +86,7 @@ func TestEjectingAPresetGivesTheBytesItAlwaysGave(t *testing.T) { | |||||||||||||||||||||||||||||||||||||
| if len(expanded.Source) == want.bytes && got == want.sum { | ||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| t.Errorf("ejecting %s at %v gives %d B and %s, and it has given %d B and %s since "+ | ||||||||||||||||||||||||||||||||||||||
| "2026-09-08.\n"+ | ||||||||||||||||||||||||||||||||||||||
| t.Errorf("ejecting %s at %v gives %d B and %s, and it has given %d B and %s until now.\n"+ | ||||||||||||||||||||||||||||||||||||||
| "Every manifest written from this preset carries a hash of these bytes, so this is a "+ | ||||||||||||||||||||||||||||||||||||||
| "breaking change under D11 rather than a number to update here.\n%s", | ||||||||||||||||||||||||||||||||||||||
| want.id, want.args, len(expanded.Source), got, want.bytes, want.sum, expanded.Source) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| package guard | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "runtime" | ||
| "testing" | ||
|
|
||
| "github.com/donislawdev/TestingFilesGenerator/internal/format" | ||
| _ "github.com/donislawdev/TestingFilesGenerator/internal/format/all" | ||
| "github.com/donislawdev/TestingFilesGenerator/internal/preset" | ||
| "github.com/donislawdev/TestingFilesGenerator/internal/recipe" | ||
| ) | ||
|
|
||
| // A new upload limit does not ask the same questions again. | ||
| // | ||
| // Expanding upload-validation took 137-142 ms and 57 MB whenever a value | ||
| // changed, which in the window is every key typed into one of its settings or | ||
| // into the batch screen built on it. Two causes, both a question asked again | ||
| // with the answer already known: the smallest size of a format, found for | ||
| // every file about a name by encoding pictures at growing sizes, and one small | ||
| // picture planned under a dozen names (docs/GUI-MEMORY-2026-09-23.md section | ||
| // 4j). Measured 2026-09-24, least of five: 9.58 MB with both answered once, | ||
| // 18.95 MB with the picture planned under every name, 44.29 MB with the floor | ||
| // worked out for every file, 9.67 MB under -race. The line sits between the | ||
| // first two. | ||
| // | ||
| // Asked with a limit this process has not expanded before, because the same | ||
| // limit twice is the window's memory's business and says nothing about this. | ||
| // The least of several readings, because the counter is the whole process's | ||
| // and a reading can only be too high - see | ||
| // TestTheMinimalSetIsWorkedOutOnceAndNotAtEveryExpansion. | ||
| // | ||
| // If it goes red after a change that is not about this, measure the four | ||
| // numbers above again before moving the line: a line moved to the new reading | ||
| // no longer stands between anything. | ||
| func TestANewUploadLimitDoesNotAskTheSameQuestionsAgain(t *testing.T) { | ||
| const ceiling = 13<<20 + 1<<19 // 13.5 MB | ||
| // The state this is about: files that ask one question under several | ||
| // names. A set without them would expand cheaply whatever this code did. | ||
| if most := mostFilesAskingOneQuestion(t, "upload-validation", preset.Args{}); most < 5 { | ||
| t.Fatalf("the most files of upload-validation asking one question is %d, so the set no longer holds what this guard is about", most) | ||
| } | ||
| least := leastAllocatedByAnExpansion(t, "upload-validation", func(i int) preset.Args { | ||
| return preset.Args{"limit": fmt.Sprintf("%dmb", 21+i)} | ||
| }) | ||
| if least > ceiling { | ||
| t.Errorf("expanding upload-validation at a new limit allocated %d bytes, over %d - "+ | ||
| "a format's smallest size or a file already planned is being worked out again", least, ceiling) | ||
| } | ||
| } | ||
|
|
||
| // A new row count works the sheet's smallest size out once, not twice. | ||
| // | ||
| // tabular-import asks for its sheet at the smallest size the rows and columns | ||
| // allow, and finding that size means building the sheet at growing sizes. The | ||
| // size was asked twice per expansion - once to check the file, once to write | ||
| // it into the set - and nothing remembered it. A new row count does change the | ||
| // sheet, so one working out is the real work and the second was the waste. | ||
| // The wide CSV, the other file asked at its floor, is the same at every | ||
| // expansion and now comes from memory. | ||
| // | ||
| // Measured 2026-09-24 at 300 rows, least of five: 19.84 MB remembered, 38.59 MB | ||
| // worked out twice, 20.63 MB under -race. The line sits between. The numbers | ||
| // grow with the rows - 700 rows allocate 39 MB remembered - so the rows asked | ||
| // here stay where they were measured. | ||
| func TestANewRowCountWorksTheSheetsSmallestSizeOutOnce(t *testing.T) { | ||
| const ceiling = 27 << 20 | ||
| if sheets := filesOfFormat(t, "tabular-import", preset.Args{}, "xlsx"); sheets != 1 { | ||
| t.Fatalf("tabular-import holds %d sheets, so the set no longer holds the file this guard is about", sheets) | ||
| } | ||
| least := leastAllocatedByAnExpansion(t, "tabular-import", func(i int) preset.Args { | ||
| return preset.Args{"rows": fmt.Sprintf("%d", 300+i)} | ||
| }) | ||
| if least > ceiling { | ||
| t.Errorf("expanding tabular-import at a new row count allocated %d bytes, over %d - "+ | ||
| "the sheet's smallest size is being worked out more than once", least, ceiling) | ||
| } | ||
| } | ||
|
|
||
| // leastAllocatedByAnExpansion expands a preset once, then five times with | ||
| // values it has not been given, and returns the least any of the five | ||
| // allocated. | ||
| func leastAllocatedByAnExpansion(t *testing.T, id string, fresh func(int) preset.Args) uint64 { | ||
| t.Helper() | ||
| if _, err := preset.Expand(id, preset.Args{}); err != nil { | ||
| t.Fatalf("%s did not expand, so nothing was asked: %v", id, err) | ||
| } | ||
| least := ^uint64(0) | ||
| for i := 0; i < 5; i++ { | ||
| var before, after runtime.MemStats | ||
| runtime.ReadMemStats(&before) | ||
| if _, err := preset.Expand(id, fresh(i)); err != nil { | ||
| t.Fatalf("%s at %v: %v", id, fresh(i), err) | ||
| } | ||
| runtime.ReadMemStats(&after) | ||
| if spent := after.TotalAlloc - before.TotalAlloc; spent < least { | ||
| least = spent | ||
| } | ||
| } | ||
| return least | ||
| } | ||
|
|
||
| // mostFilesAskingOneQuestion is how many targets of an expanded set ask their | ||
| // format the same thing, at most - one format, one size, one set of settings. | ||
| func mostFilesAskingOneQuestion(t *testing.T, id string, args preset.Args) int { | ||
| t.Helper() | ||
| same := map[string]int{} | ||
| most := 0 | ||
| for _, target := range expandedTargets(t, id, args) { | ||
| if len(target.Sizes) == 0 { | ||
| continue | ||
| } | ||
| key, ok := format.RequestKey(target.Format, format.Request{ | ||
| Label: true, Properties: target.Properties, Bytes: target.Sizes[0], | ||
| }) | ||
| if !ok { | ||
| continue | ||
| } | ||
| same[key]++ | ||
| if same[key] > most { | ||
| most = same[key] | ||
| } | ||
| } | ||
| return most | ||
| } | ||
|
|
||
| func filesOfFormat(t *testing.T, id string, args preset.Args, formatID string) int { | ||
| t.Helper() | ||
| n := 0 | ||
| for _, target := range expandedTargets(t, id, args) { | ||
| if target.Format == formatID { | ||
| n++ | ||
| } | ||
| } | ||
| return n | ||
| } | ||
|
|
||
| func expandedTargets(t *testing.T, id string, args preset.Args) []recipe.Target { | ||
| t.Helper() | ||
| expanded, err := preset.Expand(id, args) | ||
| if err != nil { | ||
| t.Fatalf("%s did not expand: %v", id, err) | ||
| } | ||
| rec, err := recipe.Parse(expanded.Source, id) | ||
| if err != nil { | ||
| t.Fatalf("%s expanded into a recipe that does not read: %v", id, err) | ||
| } | ||
| return rec.Targets | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The heading claims a speedup for every preset, but the text measures only
upload-validation.The bold line says "Changing a setting of a preset is about four times faster." The body only gives numbers for
upload-validation. The PR's own measurements show a smaller gain elsewhere. Fortabular-import,internal/guard/presetcost_test.gorecords 38.59 MB going down to 19.84 MB, which is about two times. A user oftabular-importortext-encodingwould read the heading as a promise the change does not keep. Name the preset in the heading.Proposed fix
As per path instructions: "Flag ... entries that do not match what the PR actually changes." Also: "Text must agree with the state it describes."
🤖 Prompt for AI Agents
Source: Path instructions