Assert profile-path attribution on call counts, not elapsed time - #128
Merged
Conversation
ProfilePaths.SameCalleeUnderTwoCallersGetsSeparateNodes compared the cumulative TIME of the same callee under a light and a heavy caller. That failed a macOS CI run and blocked a merge. It is a genuine flake, not a symptom of anything: heavy does about ten times the work of light, so the ratio is normally around 9x -- but measured over 15 runs it collapsed as low as 1.79x, and on a shared runner it can cross 1.0. Wall clock is not something to assert an ordering on when an exact quantity is available. The exact quantity was already there. Recursion folds onto a child node, and that child carries the call count: 2 under light against 20 under heavy, identical on both engines. That is a stronger statement of the property the test exists for -- each path carries its own attribution -- and it cannot invert. The times are still checked, but only for existence, which is all that is deterministic about them. Checked while here: no other assertion in the file compares timings between two workloads. The rest are >= 0, > 0, or cumulative >= self, which are structural. 60 runs across both engines, no failures. 1078 tests pass.
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.
ProfilePaths.SameCalleeUnderTwoCallersGetsSeparateNodesfailed a macOS CI run and blocked the merge of #127.It is a genuine flake
The assertion compared elapsed time of the same callee under a light and a heavy caller:
EXPECT_GT(workUnderHeavy->cumulativeTime, workUnderLight->cumulativeTime);heavydoes ~10× the work, so the ratio is normally ~9×. Measured over 15 runs it collapsed as low as 1.79× — and on a shared CI runner it can cross 1.0. Wall clock is not something to assert an ordering on when an exact quantity is available.Confirmed by re-running the identical commit: it passed.
The exact quantity was already there
I first tried
callCounton the node itself — 1 for both, because recursion folds. Measuring the actual tree showed where the difference lives:The folded recursion child carries it: 2 against 20, identical on both engines. That is a stronger statement of what the test exists to prove — each path carries its own attribution — and it cannot invert.
Times are still asserted, but only for existence, which is all that is deterministic about them.
Checked while here
No other assertion in the file compares timings between two workloads. The rest are
>= 0,> 0, orcumulative >= self— structural, not races.60 runs across both engines, no failures. 1078 tests pass.
Version 0.51.1 → 0.51.2.
🤖 Generated with Claude Code