test(menubar): make the localization scan cost milliseconds, not seconds - #1340
Closed
ozymandiashh wants to merge 1 commit into
Closed
test(menubar): make the localization scan cost milliseconds, not seconds#1340ozymandiashh wants to merge 1 commit into
ozymandiashh wants to merge 1 commit into
Conversation
swift-testing runs suites concurrently in one process, so a slow test steals CPU from every wall-clock assertion running beside it. ServeConnectionTests asserts that cancelling a hung request returns inside 500 ms (getagentseal#1333), and on PR getagentseal#1289 it overshot by 16 ms twice in a row while LocalizationCoverageTests ran next to it. That scan was burning about 21 s of CPU in the unoptimised build `swift test` uses. None of that cost was essential — the tree is 88 files and 1.3 MB — it was four algorithmic mistakes: - the file was walked once per call-site pattern, 26 times over; - every comparison built a fresh Array slice at each character position, tens of millions of allocations per run; - enclosingTypeName rescanned the file from the top for every display-label property it found, which is quadratic; - and the suite's four whole-tree tests each re-read and re-tokenized every file. The scanner now blanks comments in place over a UTF-8 buffer, walks each file exactly once behind a one- and two-byte dispatch table (`.` alone is about one byte in thirty and carries ten candidate patterns), tracks the enclosing type as the walk passes it, builds the line index only when there is a finding to report, and memoises the scan so the four tests pay for one. Measured on this host, debug build as CI runs it (load average ~35 from concurrent builds, so wall time is contention; thread CPU is the stable figure): before after thread CPU, whole suite 20.6 - 22.4 s 0.20 - 0.25 s wall, same runs 133 - 200 s 0.22 - 0.86 s release build, CPU 6.2 s ~25 ms Detection is unchanged, and this is only a performance fix: - all 22 existing LocalizationCoverageTests pass unmodified against the new scanner (run through a Testing shim, since this host has no Testing module); - a differential run of main's scanner against this one over every source file, three mutations of each that force the positive paths (every L( un-routed, stripped, and so on), and 25 edge cases including nested block comments, multiline and unterminated literals and non-ASCII text, agrees on all 313 findings and 1,406 keys. A new test pins the cost: it prints the scan's wall and thread CPU once, and fails above 2 s of thread CPU — ten times the current figure, so it trips on an algorithmic regression rather than a slow runner. It asserts thread CPU, not wall time, because a wall-clock ceiling on a shared runner would be the very flake this exists to prevent, and not process CPU, which would bill it for every test running concurrently. Mutation-checked: against main's original scanner it fails at 20.6 s. Refs getagentseal#1333
Member
|
Closing. Six seconds of test time does not justify a 600-line scanner rewrite, and a CPU-budget assertion is a new flake source of its own. #1345 removes the timing dependency that made the scan cost matter, which is the fix we want. |
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
2 participants
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.
Summary
LocalizationCoverageTestswas burning ~21 s of CPU in the debug buildswift testuses. swift-testing runs suites concurrently in one process, so that CPU was taken from every wall-clock assertion running beside it — includingServeConnectionTests' 500 ms cancellation budget, which overshot by 16 ms twice in a row on feat(menubar): i18n support + Simplified Chinese localization for the macOS menubar #1289 (mac CI: ServeConnectionTests is timing-flaky under load (two different tests in two days) #1333).enclosingTypeNamerescanned from the top for every property (quadratic), and the suite's four whole-tree tests each re-read every file. The scanner now makes one byte-level pass per file behind one- and two-byte dispatch tables, tracks the enclosing type as it walks, builds the line index only when there is a finding, and memoises the scan so the four tests pay for one.scanStaysCheap, which prints the scan's cost once and fails above 2 s of thread CPU (~10× today). Thread CPU rather than wall time, so the ceiling cannot itself become a flake on a shared runner, and rather than process CPU, which would bill it for concurrently running tests. It fails at 20.6 s against the previous scanner.Detection is unchanged; this is a performance-only change. All 22 existing coverage tests pass unmodified, and a differential run of the old scanner against the new one — every source file, three mutations of each that force the positive paths, and 25 edge cases (nested comments, multiline/unterminated literals, non-ASCII) — agrees on all 313 findings and 1,406 keys.
Refs #1333
Testing
npm testpassesnpm run buildsucceedsTest-only Swift change; no TypeScript touched.
swift build --package-path macsucceeds.swift testcould not run on the machine this was prepared on (Command Line Tools only, noTestingmodule — true onmaintoo), so the suite's test bodies were run through a minimal Testing shim: 23/23 pass. Timings above were taken on a heavily loaded machine, so wall time there is contention; thread CPU is the stable figure. CI runs the real suites.