Skip to content

test: stabilize Performance_Compare{Writer,Reader}Methods#129

Merged
stevehansen merged 1 commit into
masterfrom
test/stabilize-perf-tests
Jun 7, 2026
Merged

test: stabilize Performance_Compare{Writer,Reader}Methods#129
stevehansen merged 1 commit into
masterfrom
test/stabilize-perf-tests

Conversation

@stevehansen

Copy link
Copy Markdown
Owner

@

Problem

Performance_CompareWriterMethods and Performance_CompareReaderMethods gated CI on wall-clock Stopwatch.ElapsedMilliseconds ratios (e.g. memoryTime <= traditionalTime * 3). At sub-5ms, millisecond rounding under MSTest parallelism makes those ratios trip at random — the tests flake despite no behavioral regression. Performance_CompareWriterMethods flaked once during work on #128.

Fix

Swap the measurement axis from elapsed time to GC.GetAllocatedBytesForCurrentThread() — a deterministic per-thread counter, immune to CI noise, test parallelism, and GC timing. Same code + same data yields the same number every run. This mirrors the pattern already applied to Memory_AllocationComparison.

  • Warm up all paths first so first-call JIT cost doesnt skew whichever runs first.
  • Keep the load-bearing, fully deterministic correctness checks — the writers produce byte-identical output; the readers parse to identical values.
  • Replace the flaky timing-ratio asserts with a deterministic allocation tripwire: each alternative path must stay within 2x the traditional paths allocations.
  • Drop the now-unused System.Diagnostics import.

Measured headroom

Path Allocations vs traditional
Memory writer 1.00x
Buffer writer 0.50x
Span reader 1.00x
Optimized reader 0.45x

The 2x bound is a generous regression backstop (not a fine-grained benchmark) that still catches a reintroduced per-cell allocation. The buffer writers allocation profile remains gated separately by Memory_AllocationComparison.

Verification

10 isolated writer runs + 6 PerformanceTests-class runs + 8 full-suite runs (183/183 each) — zero flakes across all of it.

Independent of #127 and #128 (neither touches PerformanceTests.cs).

🤖 Generated with Claude Code
@

Both tests gated CI on wall-clock Stopwatch.ElapsedMilliseconds ratios
(e.g. memoryTime <= traditionalTime * 3). At sub-5ms, millisecond
rounding under MSTest parallelism made those ratios trip at random, so
the tests flaked despite no behavioral regression.

Switch the measurement axis from elapsed time to
GC.GetAllocatedBytesForCurrentThread() -- a deterministic per-thread
counter, immune to CI noise, test parallelism, and GC timing. Same code
+ same data yields the same number every run. This mirrors the pattern
already applied to Memory_AllocationComparison.

- Warm up all paths first so first-call JIT cost doesn't skew whichever
  runs first.
- Keep the load-bearing, fully deterministic correctness checks (the
  writers produce byte-identical output; the readers parse to identical
  values).
- Replace the flaky timing-ratio asserts with a deterministic allocation
  tripwire: each alternative path must stay within 2x the traditional
  path's allocations. Measured reality: memory writer 1.00x, buffer
  0.50x; span reader 1.00x, optimized 0.45x -- generous headroom that
  still catches a reintroduced per-cell allocation.
- Drop the now-unused System.Diagnostics import.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors performance tests in Csv.Tests/PerformanceTests.cs to measure and compare memory allocations instead of wall-clock time, reducing test flakiness in CI environments. It also introduces a warm-up phase to eliminate JIT compilation overhead. The review feedback correctly points out that the arguments in several Assert.IsLessThanOrEqualTo calls are reversed, which inverts the assertion logic and prevents the tests from properly catching allocation regressions.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +76 to +77
Assert.IsLessThanOrEqualTo(traditionalMemory * 2, memoryAllocated,
$"Memory writer allocations should be competitive with the traditional writer (traditional: {traditionalMemory:N0}, memory: {memoryAllocated:N0})");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The arguments to Assert.IsLessThanOrEqualTo are reversed. In MSTest, the signature is Assert.IsLessThanOrEqualTo(actual, expected, message). Passing traditionalMemory * 2 as the first argument asserts that traditionalMemory * 2 <= memoryAllocated, which is the opposite of the intended check (that memoryAllocated <= traditionalMemory * 2). This reverses the test logic and prevents it from catching allocation regressions.

            Assert.IsLessThanOrEqualTo(memoryAllocated, traditionalMemory * 2,
                $"Memory writer allocations should be competitive with the traditional writer (traditional: {traditionalMemory:N0}, memory: {memoryAllocated:N0})");

Comment on lines +136 to +139
Assert.IsLessThanOrEqualTo(traditionalMemory * 2, spanAllocated,
$"Span reader allocations should be competitive with the traditional reader (traditional: {traditionalMemory:N0}, span: {spanAllocated:N0})");
Assert.IsLessThanOrEqualTo(traditionalMemory * 2, optimizedAllocated,
$"Optimized reader allocations should be competitive with the traditional reader (traditional: {traditionalMemory:N0}, optimized: {optimizedAllocated:N0})");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The arguments to Assert.IsLessThanOrEqualTo are reversed here as well. In MSTest, the signature is Assert.IsLessThanOrEqualTo(actual, expected, message). Passing traditionalMemory * 2 as the first argument asserts that traditionalMemory * 2 <= spanAllocated (and optimizedAllocated), which is the opposite of the intended check (that the allocated bytes are less than or equal to twice the traditional memory).

            Assert.IsLessThanOrEqualTo(spanAllocated, traditionalMemory * 2,
                $"Span reader allocations should be competitive with the traditional reader (traditional: {traditionalMemory:N0}, span: {spanAllocated:N0})");
            Assert.IsLessThanOrEqualTo(optimizedAllocated, traditionalMemory * 2,
                $"Optimized reader allocations should be competitive with the traditional reader (traditional: {traditionalMemory:N0}, optimized: {optimizedAllocated:N0})");

@stevehansen stevehansen merged commit 3fe4344 into master Jun 7, 2026
3 checks passed
@stevehansen stevehansen deleted the test/stabilize-perf-tests branch June 7, 2026 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant