Skip to content

Daily Perf Improver - Test Parallelization Guidance and Performance Analysis#3682

Merged
pelikhan merged 1 commit into
mainfrom
perf/parallelize-mcp-add-integration-test-04d95a48e7e63c29
Nov 12, 2025
Merged

Daily Perf Improver - Test Parallelization Guidance and Performance Analysis#3682
pelikhan merged 1 commit into
mainfrom
perf/parallelize-mcp-add-integration-test-04d95a48e7e63c29

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

Goal and Rationale

Investigated Priority 1 performance goal: "Test Suite Performance (25s → <20s target)". Through hands-on measurement and optimization attempts, discovered that test suite is already well-optimized and identified a critical anti-pattern for test parallelization.

Approach

  1. Measured current test performance using make test-unit and make test-perf
  2. Identified slowest test (TestMCPAddIntegration_AddAllServers at 2.0s)
  3. Attempted parallelization using t.Parallel() pattern from success story
  4. Discovered anti-pattern - parent-aggregates-results tests incompatible with parallelization
  5. Documented learnings in build performance guide

Impact Measurement

Current Test Performance (2025-11-12)

Baseline measurements:

  • Unit tests (fresh, no cache): 12.0s (vs 25s assumed in plan)
  • Unit tests (with cache): 5.8s
  • Conclusion: Tests already 52% faster than original baseline assumption

Parallelization attempts:

  1. TestMCPAddIntegration_AddAllServers with t.Parallel():

    • Achieved 8x speedup (2.0s → 0.25s)
    • Failed: Test structure incompatible (parent aggregates results, cleanup runs too early)
  2. Using -parallel 8 flag:

    • Result: 12.0s → 11.8s (only 2% improvement)
    • Reason: Most tests don't use t.Parallel(), flag has minimal effect

Trade-offs

What changed:

  • Added comprehensive anti-pattern documentation
  • Included concrete failing example with explanation
  • Provided solutions for similar situations

Complexity:

  • Documentation only - no code changes
  • Zero maintenance burden
  • High educational value for future optimization work

Validation

Testing approach:

  • Verified measurements with multiple test runs
  • Confirmed anti-pattern behavior through actual implementation
  • Validated documentation clarity and technical accuracy

Success criteria met:

  • ✅ Identified root cause of parallelization failure
  • ✅ Documented anti-pattern with clear explanation
  • ✅ Provided actionable solutions
  • ✅ Updated performance baseline (12s vs 25s)

Future Work

Based on this analysis, recommend focusing on more impactful Priority 1 goals:

  1. Incremental workflow compilation (2.1s → <1s for single file changes)

    • Current: Recompiles all 74 workflows even when one changes
    • Opportunity: 50-70% faster recompilation during development
  2. agent-finish optimization (10-15s → <8s)

    • Current: Sequential execution of build, test, recompile, fmt, lint
    • Opportunity: Parallelize independent operations

Performance Evidence

Test suite already optimized:

# Fresh compilation (no cache)
$ time make test-unit
real    0m12.010s
user    0m20.283s
sys     0m6.031s

# With cache
$ time make test-unit  
real    0m5.792s
user    0m6.911s
sys     0m2.489s

Original plan baseline: 25s
Actual baseline: 12s
Performance gap: Already 52% better than assumed baseline

Reproducibility

To reproduce performance measurements:

# Clean cache and measure
go clean -testcache
time make test-unit

# Identify slow tests
make test-perf

# Test with parallel flag
go clean -testcache  
time go test -v -timeout=3m -parallel 8 -tags '!integration' ./...

References

AI generated by Daily Perf Improver

Document the parent-aggregates-results anti-pattern discovered during
performance optimization work. This pattern is incompatible with t.Parallel()
because:

1. Parallel subtests pause until parent function returns
2. Parent's defer cleanup() runs before subtests resume
3. Parent aggregates results before subtests execute

Adds concrete example and solutions to help future performance engineering
efforts avoid this pitfall.
@pelikhan pelikhan marked this pull request as ready for review November 12, 2025 04:02
Copilot AI review requested due to automatic review settings November 12, 2025 04:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR documents a critical anti-pattern discovered during test parallelization optimization efforts. The investigation revealed that the test suite is already performing better than assumed (12s vs 25s baseline), and identified a specific scenario where t.Parallel() cannot be safely applied.

Key Changes:

  • Added comprehensive documentation of the "Parent-Aggregates-Results" anti-pattern for Go test parallelization
  • Provided clear explanation of why t.Parallel() fails in tests where parent functions aggregate results from subtests
  • Included concrete code example with detailed failure analysis and solutions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pelikhan pelikhan merged commit 1b86ac2 into main Nov 12, 2025
10 checks passed
@pelikhan pelikhan deleted the perf/parallelize-mcp-add-integration-test-04d95a48e7e63c29 branch November 12, 2025 04:21
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.

2 participants