Repository navigation
performance: keep results measured before a history reset out of the series - #587
Open
Jay (jay23606) wants to merge 4 commits into
Open
Jay (jay23606) wants to merge 4 commits into
Jay (jay23606) wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Legacy CSV parsing and merged-history comparisons can retain stale measurements or discard unaffected results.
2 open findings
What changed in this PR
Addresses #511 by filtering delayed benchmark results that predate a history reset, protecting the performance gate’s ten-point warmup.
Changes:
- Adds Git-history reset detection to persistence.
- Adds a selective-reset regression test.
- Documents the stale-result exclusion rule.
| File | Description |
|---|---|
| scripts/test_performance.py | Tests delayed results after a selective reset. |
| scripts/nvx_tools/performance.py | Filters pre-reset results using committed history. |
| doc/benchmarks.md | Clarifies persistence behavior after resets. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Jay (jay23606)
force-pushed
the
fix/persist-results-after-history-reset
branch
from
October 19, 2026 00:07
cb054a3 to
a640846
Compare
Jay (jay23606)
force-pushed
the
fix/persist-results-after-history-reset
branch
from
October 20, 2026 05:20
a640846 to
f15f9f8
Compare
Jay (jay23606)
force-pushed
the
fix/persist-results-after-history-reset
branch
from
October 20, 2026 11:27
980cdf1 to
32c1e89
Compare
Jay (jay23606)
force-pushed
the
fix/persist-results-after-history-reset
branch
from
October 20, 2026 11:50
32c1e89 to
ce79af6
Compare
…series persist_results appended any row whose commit and metric were absent from the destination CSV. A reset removes those identities, so a result measured before a reset commit was inserted after it and counted toward the ten-point warmup. A delayed dev run could therefore gate a metric that had only nine post-reset measurements. Before appending, persist now asks git which dimensions lost rows between each result's commit and HEAD, and skips those rows. A result whose commit is not an ancestor of HEAD fails the persist instead of being guessed. Unaffected metrics still persist. The CLI checks whenever its history directory is inside a git checkout, which the persist job always is. Adds a regression test in which a result from before a reset arrives after nine post-reset rows; the series stays at nine rows and the unaffected throughput row still persists. Documents the rule next to the reset contract. Fixes microsoft#511 (performance history reset race) Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Signed-off-by: jay23606 <4602056+jay23606@users.noreply.github.com>
…atform The reset check compared consecutive commits in log order. A side branch forked before the measurement can hold fewer rows without resetting them, so it was misread as a reset. Compare each commit with its own parents instead, and include merged history with --full-history. A legacy CSV takes its platform from its file name, so the committed blob keeps its basename when it is read back. Adds a regression test in which a side branch changes latency without throughput and is merged with A's throughput row intact. Before this change the check flagged throughput. Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Signed-off-by: jay23606 <4602056+jay23606@users.noreply.github.com>
…t's line A branch forked before an earlier reset can keep rows that the target already reset before the measurement. Merging it drops those rows relative to that branch, which was misread as a reset after the measurement, and its valid result was discarded. Compare a commit with a parent only when the parent came before the measurement or was built on it. Adds a regression test for that merge. Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Signed-off-by: jay23606 <4602056+jay23606@users.noreply.github.com>
Jay (jay23606)
force-pushed
the
fix/persist-results-after-history-reset
branch
from
October 20, 2026 11:59
ce79af6 to
9e8b746
Compare
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
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.
Fixes #511 (performance history reset race).
Summary
persist_resultsappended any result whose commit and metric were missing from the destination CSV. A reset removes those identities, so a result measured before a reset commit was inserted after it and counted toward the ten-point warmup. A delayeddevrun could then gate a metric with only nine post-reset measurements.HEAD, and skips those rows. A result whose commit is not an ancestor ofHEADfails the persist rather than being guessed at. Unaffected metrics still persist.fetch-depth: 0, so the history is available. Callers outside a checkout keep the previous behavior.doc/benchmarks.mdnow states the rule.Test
test_persist_drops_rows_measured_before_a_later_history_resetuses a real temporary git repository. It persists a measurement from commit A, commits a reset of the latency metric only, adds nine post-reset rows, and then persists the stale A rows. The latency series keeps nine rows, and the unaffected throughput row from A is still persisted. The test failed before the change with aTypeErroron the new argument.python -B -m unittest test_nvx_tools: 538 tests, OK (54 skipped, as before).test_performance: 84 tests, OK. ruff check and format are clean.devpersist with the workflow's retry loop.🤖 Generated with Claude Code