Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new solver variant (AutogramBytesNoStringsV8) that updates computedCounts incrementally (delta-based) rather than fully recomputing each iteration, and wires it into tests and benchmarks.
Changes:
- Introduce
AutogramBytesNoStringsV8with delta-based computed-count updates and debug verification. - Add an xUnit test case for V8 to ensure it still finds a valid autogram with the expected iteration count.
- Update benchmarks to include V8 and comment out several older benchmark variants.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| AutogramTest/AutogramTest.cs | Adds TestV8() to validate the new solver implementation. |
| AutogramBenchmark/AutogramBenchmark.cs | Adds V8 benchmark wiring and comments out multiple older benchmark variants. |
| Autogram/AutogramBytesNoStringsV8.cs | New V8 solver using delta updates for computedCounts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #if DEBUG | ||
| var test = ReCalculateComputedCounts(); | ||
|
|
||
| Debug.Assert(test.SequenceEqual(computedCounts)); | ||
|
|
There was a problem hiding this comment.
The DEBUG-only verification in DeltaUpdateComputedCounts calls ReCalculateComputedCounts() every iteration, allocating a new array and doing an O(n^2) recompute. With the existing tests running ~1.7M iterations, this can severely slow/debug builds and may cause excessive GC or timeouts. Consider guarding this behind a dedicated compilation symbol, sampling (e.g., every N iterations), or reusing a buffer to avoid per-iteration allocations.
| private List<AutogramBytesNoStringsV5b> solver5bList = null!; | ||
| private List<AutogramBytesNoStringsV6> solver6List = null!; | ||
| private List<AutogramIntsNoStringsV7> solver7List = null!; | ||
| private List<AutogramBytesNoStringsV8> solver8List = null!; |
There was a problem hiding this comment.
This benchmark class now has several solver list fields (e.g., V5a/V5b/V6/V7) that are no longer referenced because the corresponding setup/cleanup/benchmark methods are commented out. These become dead code and can introduce warnings/confusion; consider removing the unused fields or re-enabling the benchmarks via BenchmarkDotNet categories/filters instead of commenting them out.
| private List<AutogramBytesNoStringsV8> solver8List = null!; |
| //[IterationSetup(Targets = new[] { nameof(AutogramBytesNoStringsV4_Solve_Average_Batched_Seeds) })] | ||
| //public void IterationSetupV4() | ||
| //{ | ||
| // solver4List = CreateSolvers(SeedCount, p => new AutogramBytesNoStringsV4(autogramConfig, p)); | ||
| //} | ||
|
|
There was a problem hiding this comment.
Large commented-out IterationSetup blocks make it harder to tell which benchmarks are intended to run and are easy to forget to re-enable. Prefer deleting unused benchmark variants, or using BenchmarkDotNet mechanisms (categories, filters, or config) / conditional compilation to control which benchmarks execute instead of leaving commented code in the source.
| //[IterationSetup(Targets = new[] { nameof(AutogramBytesNoStringsV4_Solve_Average_Batched_Seeds) })] | |
| //public void IterationSetupV4() | |
| //{ | |
| // solver4List = CreateSolvers(SeedCount, p => new AutogramBytesNoStringsV4(autogramConfig, p)); | |
| //} |
|
|
||
| if (reorderedEquals) | ||
| { | ||
| var wasReordered = computedCounts.AsSpan().SequenceEqual(proposedCounts) == false; |
There was a problem hiding this comment.
The expression 'A == false' can be simplified to '!A'.
| var wasReordered = computedCounts.AsSpan().SequenceEqual(proposedCounts) == false; | |
| var wasReordered = !computedCounts.AsSpan().SequenceEqual(proposedCounts); |
| : OffsetGuess(computedCount, variableMinimumCount[i], randomizationLevel); | ||
| } | ||
|
|
||
| if (history.Contains(result) == false) |
There was a problem hiding this comment.
The expression 'A == false' can be simplified to '!A'.
| if (history.Contains(result) == false) | |
| if (!history.Contains(result)) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Attempt to improve calculation performance. No significant gains.