feat(perf): assemble the performance data path, in the one package that can see all four - #14
Conversation
…at can see all four dusk reports numbers produced by telescope, wind and magic, and its frozen dependency contract forbids it from importing any of them. This package is the only place in the ecosystem where all four are visible at once, so this is where the seam gets closed: four settable pointers dusk declares with no-op defaults, assigned here to the real sources. The failure this class exists to prevent is silent. Every pointer's default is structurally complete rather than null, which is what lets both sides compile independently, and also what means an unassigned one produces a report of zeros instead of an error, in a different repository, at the end of a driven run that looked like it worked. So the tests seed the store, install, and read back THROUGH each pointer rather than asserting that install() did not throw. Ordering is load-bearing in two places. The observer registers before the idempotency guard is armed, because MagicRouter.addObserver throws once the router has been built and marking the integration installed first would turn a legitimate retry into a silent no-op. And the whole install belongs in installPre, ahead of Magic.init, for the same reason: the throw is deliberately not caught, since a swallowed one leaves the report with no route transitions and nothing to explain their absence. The session hooks are scoped rather than global. Begin zeroes wind's counters, turns counting on, clears telescope's frame buffer and clears the magic-side counters; end turns counting back off. Without the magic-side clear the magic section would report the sum of every previous session while the wind and frame sections reported only the current one, which is the kind of number that is worse than no number. clearFramePerf() rather than clear(), so the HTTP, log and exception buffers a developer may be reading alongside the session survive it.
…ese repos gates on CI runs `dart format --output=none --set-exit-if-changed` in both ci.yml and publish.yml here, and this branch had never had the formatter run on it. Ten files across four repos were dirty, so four of the PRs would have gone red on a check that was never part of my verification loop. The omission has a specific cause worth naming. The consumer app these packages were driven from carries a standing rule NOT to run dart format, because its tree predates the current SDK's tall formatter and reformatting rewrites dozens of untouched files. That rule is about that repository. I carried it into these six, where it does not apply: they resolve to short style in-repo and they gate on a zero diff. A project-specific rule applied outside its project. Only branch-introduced files were formatted, so nothing untouched moved.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. The wiring itself reads correctly and the tests target the right thing (data through each pointer, not Critical
Evidence, on this head with a clean Minor
Tests
Checks I ran
|
…ons being ranked Four review points, all correct. The idempotency guard was armed after the observer registration but before the four steps that follow it, so a throw from Wind.installPerfResolver(), registerWatcher or the pointer assignments left the guard set with the pointers unassigned, and any retry became exactly the silent no-op the class exists to prevent. It is armed at the end now, and the observer gets its own flag so a retry does not register a second one. The original comment argued for the right property and the code only delivered half of it. Route transitions no longer record anonymous pushes. showDialog and showModalBottomSheet go through the same navigator, so in a dialog-heavy session those entries shared the bounded list with real page transitions and could evict the ones the report is ranking. Skipped rather than bucketed: a duration nobody can attribute to a screen is not one an agent can act on. resetForTesting no longer hand-writes dusk's no-op defaults. It captures them at load, before this package assigns over them, and restores those. Re-typing them let this package and its own tests agree on a key set that had drifted from dusk's, so the assertions would have kept passing while production drifted with them. And installPre's docstring gained the caveat it needed. It ends "a second call in the same isolate is safe", which is still true, but a FIRST call made after the router is built now throws. A host installing behind a lazy debug toggle after runApp used to get harmless no-ops.
|
All four Minors fixed in The guard armed before steps 2 to 4. You are right and this one bothered me, because the comment sitting above it argued for exactly the property the code did not deliver. Anonymous pushes. Fixed by skipping them rather than bucketing them.
The Critical. The constraints do need to move, but not for the reason given:
I checked this, because it decides whether this is a release-ordering matter or a dead end:
What survives, and what I am acting on, is the half that does not depend on that: the floors should state what the code actually needs, so pub cannot legally resolve a version lacking the API and a published You are also right that
|
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of Since my last review: guard armed last with a separate observer flag, anonymous pushes skipped plus a test, dusk defaults captured instead of hand-written, Major
It passes today only because Minor
TestsThe new Checks I ran
|
The previous commit claimed to capture dusk's defaults "before this package assigns over them" and did not. A top-level `final` in Dart initialises on first READ, and the only reader is resetForTesting(), which runs after install() has already overwritten the pointers. So it captured this package's own closures and restored those, which means every "back to the default" assertion was really asserting that install had happened. Verified with a standalone repro of the pattern before replacing it: the reset returned INTEGRATION, not the default. A comment asserting a language behaviour the language does not have is worse than no comment, because it stops the next reader checking. Captured eagerly now, at the top of the pointer assignments, which is the last moment they are still readable. The nullable fields make "install never ran" representable, and in that case there is nothing to put back. The two tests that moved wind's counters through record* now build a real WDiv instead. Those entry points are @internal to fluttersdk_wind, so reaching for them asserted against a surface no consumer is meant to touch, and a pump is the honest version of that setup anyway: it is what moves these numbers in an app. Mutation-checked: removing the capture turns the reset test red.
|
Fixed in void Function() ptr = () => print('DUSK DEFAULT');
final void Function() captured = ptr;
void main() { ptr = () => print('INTEGRATION'); ptr(); ptr = captured; ptr(); }
// INTEGRATION
// INTEGRATIONA top-level Captured eagerly now, at the top of the pointer assignments, which is the last moment they are still readable. The fields are nullable so "install never ran" is representable, and in that case there is nothing to put back. Mutation-checked: removing the capture turns the reset test red, which the previous version would not have done. One knock-on worth flagging, since it crosses into the sibling PR. Marking wind's
|
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of Since my last review: the four defaults are captured with Major
Minor
TestsThe Checks I ran
|
Review found the two session tests coupled to wind's static parse cache, and reproducing it turned up a second order dependence underneath. WindParser._styleCache is a static map shared across the isolate, and both tests built the same WDiv under the same theme, so they shared one key. The first ran against a cold cache, which made its `expect(cacheHits, 0)` assert a value that was already 0 and prove nothing about the reset it was there to check; the second only passed because the first had warmed the key. Verified: `--plain-name 'the session pair'` alone was red with Expected 1, Actual 0. Each test now warms the cache itself through a helper that clears it, builds, and zeroes the counters. The first also asserts a non-zero BEFORE the hook, so the reset has something to have cleared. The helper pumps a different tree between the two builds: pumping an identical tree does not rebuild it, so the measured build parsed nothing and the hit never happened. That cost a wrong first attempt. The second dependence: MagicDevtools.installPre() installs telescope's DumpWatcher, which replaces the global debugPrint, and nothing put it back. So Flutter's own post-test check fired on whichever testWidgets case ran NEXT, blaming an innocent test under only some orderings. tearDown restores it now. The whole suite is green under five shuffle seeds; before, seed 12345 was red. Also corrects the restore comment, which still said the defaults are "captured once at load" after the fix moved that to first install, and drops the analysis_options exclude block: none of those seven directories exist in this package, so it excluded nothing and was unrelated to the perf path.
|
Handled in 4f42c58. The Major was right, and reproducing it turned up a second order dependence sitting underneath it. The parse-cache coupling. Confirmed before changing anything: Each test now pins its own cache state through a helper that clears the cache, builds, and zeroes the counters. The first also asserts a non-zero before the hook runs, so the reset has something to have cleared, which restores what the old One thing worth recording, because my first attempt at the helper was wrong: pumping an identical widget tree does not rebuild it, so the measured build parsed nothing and the hit never happened. The helper pumps a The second one, which your finding led me to. With the cache pinned, the suite still failed under The suite is green under five shuffle seeds (12345, 777, 4242, 99, 31337), 110 tests each. Before this commit, 12345 was red. The restore comment. Corrected. It still said "captured once at load" after the fix moved the capture to first install, and it now says why: a top-level
On your analyze note: |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of Since my last review: Minor
TestsThe two session cases are now genuinely order-independent, and Checks I ran
|
…erated yaml
Two review notes, both verified against the source before acting.
The debugPrint restore was in tearDown, which runs AFTER Flutter's end-of-test
check: AutomatedTestWidgetsFlutterBinding.runTest calls _verifyInvariants()
immediately after `await testBody()` (flutter_test/lib/src/binding.dart:1974),
so tearDown and addTearDown are both too late. It passed only because the sole
installPre() call sits in a plain test(), which has no invariant check. Proven
by converting that case to testWidgets as a probe: it failed on itself with
"the value of a foundation debug variable was changed by the test".
The restore now happens inline at the end of that body. Re-running the same
probe, the foundation error is gone. What surfaces instead is a SECOND
end-of-test invariant: installPre() leaves a SemanticsHandle active, because
dusk's snapshot pipeline enables semantics. That is latent today, since the
case is a plain test(), so it is recorded as a note rather than fixed here.
The tearDown restore stays as a net for the plain test() cases.
Reverting the analysis_options.yaml removal. The exclude block is written by
`flutter pub get` ("Upgrading analysis_options.yaml to exclude build and
platform directories"), reproduced here, so removing it just makes every tree
dirty after a dependency fetch. My earlier note that it was unrelated to this
PR was right for the wrong reason: it is unrelated because Flutter generated
it, which is exactly why it should stay.
Suite green under three shuffle seeds, 110 tests each.
|
Both handled in 077abf6, and the second one was right in a way I had not seen: my fix was passing for the wrong reason. The The restore is inline at the end of that body now. Re-running the same probe, the foundation error is gone. And the probe found a second one. With
So the block belongs to the tooling and removing it just dirties every tree after a fetch. Worth noting this also explains something from earlier in this work: I saw the same block appear in On the red CI, since it is the obvious question. It is not this PR's code. Suite green under three shuffle seeds (12345, 777, 4242), 110 tests each. |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of Since my last review: Minor
TestsNo assertions changed in this range - the Checks I ran
|
The block describing MagicPerfIntegration and the block describing the four captured dusk defaults had merged into one, sitting on the private top-level variables. The tell is mid-paragraph: a sentence about reaching wind through magic's barrel is followed, with no break, by "dusk's own no-op defaults, captured the first time...", so the class's own documentation read as an aside about four private fields and the class itself carried none. Split back to where each belongs. The class doc is on the class, the pointer doc is on the pointers. resetForTesting's comment went with it. It had absorbed a copy of the top-level final explanation inside a parenthetical, which left the surrounding sentence reading "captured on the first install rather than at load (...), rather than hand-written here". It now states its own point and refers to the fields for the rest. Comments only, no behaviour change.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of Since my last review: the TestsNo test file in this range, so no assertions changed. The two Minors from the previous round ( Checks I ran
|
…the wrong line Both carried over from the previous review round. The note explaining why the restore runs inside the test body pointed at [_restoreDebugPrint]; the function is declared nine lines below as restoreDebugPrint, with no underscore, and the call sites spell it that way too. The analyzer does not resolve doc links to a local function, so nothing was going to catch it, in the one comment whose job is to point at the fix. In the installPre case, "Inline, not in a tearDown" sat directly above an addTearDown call, which is the opposite of what it says. The inline restore is the last line of that body and the tearDown is a net; the comment now says so in that order. Also collapsed `(_, __, ___)` to `(_, _, _)` in the pageBuilder, the two unnecessary_underscores infos CI reports on this branch. They do not fail the gate (analyze runs --no-fatal-infos here) but they are the only two in the package. Comments and wildcard names only; `flutter test` is 110 green.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
What
MagicPerfIntegration, installed fromMagicDevtools.installPre(). It sets magic's notify hook, registers a route-transition observer, installs wind's perf resolver, registers telescope's frame watcher, and assigns all four of dusk's pointers.Why
dusk reports numbers produced by telescope, wind and magic, and its frozen dependency contract forbids it from importing any of them. This package is the only place in the ecosystem where all four are visible at once, so this is where the seam gets closed.
The failure this class exists to prevent is silent. Every pointer's default is structurally complete rather than null, which is what lets both sides compile independently, and also what means an unassigned one produces a report of zeros instead of an error, in a different repository, at the end of a driven run that looked like it worked. So the tests seed the store, install, and read back THROUGH each pointer rather than asserting that
install()did not throw.Ordering is load-bearing in two places. The observer registers before the idempotency guard is armed, because
MagicRouter.addObserverthrows once the router has been built and marking the integration installed first would turn a legitimate retry into a silent no-op. And the whole install belongs ininstallPre, ahead ofMagic.init(), for the same reason: the throw is deliberately not caught, since a swallowed one leaves the report with no route transitions and nothing to explain their absence.Verified rather than assumed:
MagicRouter._instanceis assigned only by its lazy getter and nulled only by a reset with no production caller, so an observer registered beforeMagic.init()survives to the routerMagic.init()populates.The session hooks are scoped, not global. Begin zeroes wind's counters, turns counting on, clears telescope's frame buffer and clears the magic-side counters; end turns counting back off. Without the magic-side clear the magic section would report the sum of every previous session while the other sections reported only the current one, which is worse than no number.
clearFramePerf()rather thanclear(), so the HTTP, log and exception buffers a developer is reading alongside the session survive it.Testing
flutter test: 109 passing.flutter analyze: clean.dart format --set-exit-if-changed: clean.Mutation-checked during development: dropping
framePerfReaderturns the seam test red withExpected: an object with length of <2> Actual: []; dropping the idempotency guard produces two observers.Depends on
All five siblings, and it is the last one to merge. It calls
MagicController.onRefreshUI(magic),Wind.installPerfResolver()(wind),FramePerfWatcherandclearFramePerf()(telescope), and assigns the four pointers (dusk), which in turn need the contract fromwind_diagnostics_contracts.Note for CI: this package's
pubspec.yamlstill pins the released versions of its five siblings, and everything this change calls is unreleased and reachable only throughpubspec_overrides.yaml. CI will not resolve until the siblings publish. That is the release train, not a defect in this change.Siblings, in merge order:
wind_diagnostics_contracts, thenmagic/fluttersdk_telescope/fluttersdk_wind/fluttersdk_dusk, then this.