From 4553f8afad3ff722666bfbce2d7b04404c470d03 Mon Sep 17 00:00:00 2001 From: AJ Isaacs Date: Fri, 25 Sep 2026 21:45:03 -0400 Subject: [PATCH] perf(fill): avoid redundant bounds recomputation --- AGENTS.md | 4 +- OpenNest.Core/Part.cs | 1 + OpenNest.Core/PerfCounters.cs | 6 + OpenNest.Engine/Fill/FillExtents.cs | 9 +- OpenNest.Engine/Strategies/FillHelpers.cs | 2 + OpenNest.Tests/Fill/FillExtentsTests.cs | 190 +++++++++++++++++- OpenNest.Tests/Fill/FillPerformanceTests.cs | 60 ++++++ OpenNest.Tests/Strategies/FillHelpersTests.cs | 117 ++++++++++- README.md | 2 + docs/performance/fill-performance.md | 104 ++++++++++ 10 files changed, 483 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8a56c72..6c0bff5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -33,7 +33,7 @@ OPENNEST_RUN_FILL_PERF=1 dotnet test OpenNest.Tests/OpenNest.Tests.csproj -c Rel Only the exact value `1` enables these tests; otherwise they skip. PowerShell: set `$env:OPENNEST_RUN_FILL_PERF = '1'`, run `dotnet test`, then `Remove-Item Env:OPENNEST_RUN_FILL_PERF`. -The category covers comparer, group-pattern, and extents-column workloads. Individual filters: `FullyQualifiedName~GroupPattern_ReportsDefaultAndCustomComparer` or `FullyQualifiedName~Extents_ReportsRepeatedColumnRebuilds`. Keep harness, inputs, warmups and batches identical before/after; exclude setup/assertions from timing. Comparer/extents allocations are synchronous and current-thread only; parallel group fills omit allocation totals. No timing CI gates or whole-job speedup claims. Preserve evidence in [the measured report](docs/performance/fill-performance.md). +The category covers comparer, group-pattern, rotated-pattern, and extents-column workloads; individual filters match benchmark method names in `FillPerformanceTests.cs`. Keep harness, inputs, warmups and batches identical before/after; exclude setup/assertions from timing. Comparer/extents allocations are synchronous and current-thread only; parallel group fills omit allocation totals. No timing CI gates or whole-job speedup claims. Preserve evidence in [the measured report](docs/performance/fill-performance.md). Debug behavior/skipped-work checks: @@ -42,7 +42,7 @@ dotnet test OpenNest.Tests/OpenNest.Tests.csproj -c Debug \ --filter 'FullyQualifiedName~DefaultFillComparerWorkTests|FullyQualifiedName~FillHelpersTests|FullyQualifiedName~FillExtentsTests|FullyQualifiedName~StrategyOverlapTests' ``` -`PerfCounters.FillScoreComputations` and `PartBoundaryPreparations` increments compile away in Release: zero Release counters prove nothing. Serialize counter assertions in `FillCacheCollection` and reset in `finally`. Keep `OpenNest.Tests/Fill/LegacyFillExtents.cs` frozen for differential tests, not production or before timings; measure the actual baseline production code. +`PerfCounters.FillScoreComputations`, `PartBoundaryPreparations`, and `PartBoundsUpdates` increments compile away in Release: zero Release counters prove nothing. Serialize counter assertions in `FillCacheCollection` and reset in `finally`. Keep `OpenNest.Tests/Fill/LegacyFillExtents.cs` frozen for differential tests, not production or before timings; measure the actual baseline production code. ## Architecture diff --git a/OpenNest.Core/Part.cs b/OpenNest.Core/Part.cs index d1c6199..5d99e87 100644 --- a/OpenNest.Core/Part.cs +++ b/OpenNest.Core/Part.cs @@ -206,6 +206,7 @@ namespace OpenNest /// public void UpdateBounds() { + PerfCounters.CountPartBoundsUpdate(); BoundingBox = Program.BoundingBox(); BoundingBox.Offset(Location); } diff --git a/OpenNest.Core/PerfCounters.cs b/OpenNest.Core/PerfCounters.cs index 08b59e7..5035dab 100644 --- a/OpenNest.Core/PerfCounters.cs +++ b/OpenNest.Core/PerfCounters.cs @@ -14,12 +14,14 @@ namespace OpenNest private static long partIntersects; private static long fillScoreComputations; private static long partBoundaryPreparations; + private static long partBoundsUpdates; public static long FindBestFits => Interlocked.Read(ref findBestFits); public static long OffsetPerimeterEntities => Interlocked.Read(ref offsetPerimeterEntities); public static long PartIntersects => Interlocked.Read(ref partIntersects); public static long FillScoreComputations => Interlocked.Read(ref fillScoreComputations); public static long PartBoundaryPreparations => Interlocked.Read(ref partBoundaryPreparations); + public static long PartBoundsUpdates => Interlocked.Read(ref partBoundsUpdates); [Conditional("DEBUG")] public static void CountFindBestFits() => Interlocked.Increment(ref findBestFits); @@ -37,6 +39,9 @@ namespace OpenNest [Conditional("DEBUG")] public static void CountPartBoundaryPreparation() => Interlocked.Increment(ref partBoundaryPreparations); + [Conditional("DEBUG")] + public static void CountPartBoundsUpdate() => Interlocked.Increment(ref partBoundsUpdates); + public static void Reset() { Interlocked.Exchange(ref findBestFits, 0); @@ -44,6 +49,7 @@ namespace OpenNest Interlocked.Exchange(ref partIntersects, 0); Interlocked.Exchange(ref fillScoreComputations, 0); Interlocked.Exchange(ref partBoundaryPreparations, 0); + Interlocked.Exchange(ref partBoundsUpdates, 0); } } } diff --git a/OpenNest.Engine/Fill/FillExtents.cs b/OpenNest.Engine/Fill/FillExtents.cs index 4b06405..211ebdb 100644 --- a/OpenNest.Engine/Fill/FillExtents.cs +++ b/OpenNest.Engine/Fill/FillExtents.cs @@ -80,8 +80,8 @@ namespace OpenNest.Engine.Fill // Position part2 to the right of part1 at bounding box width distance. var startOffset = part1.BoundingBox.Length + part2.BoundingBox.Length + partSpacing; part2.Offset(startOffset, 0); - part2.UpdateBounds(); + // Slide uses locations, not cached bounds; Offset already translates the box. // Slide part2 left toward part1. var movingLines = boundary2.GetLines(part2.Location, PushDirection.Left); var stationaryLines = boundary1.GetLines(part1.Location, PushDirection.Right); @@ -94,7 +94,6 @@ namespace OpenNest.Engine.Fill if (dist < double.MaxValue && dist > 0) { part2.Offset(-dist, 0); - part2.UpdateBounds(); } var pair = AnchorToWorkArea(part1, part2); @@ -318,9 +317,9 @@ namespace OpenNest.Engine.Fill // Separate: shift part2 right so bounding boxes don't touch. p2.Offset(partSpacing, 0); - p2.UpdateBounds(); - // Apply the vertical shift. + // Apply the vertical shift. Recompute once after both offsets: Compactor's + // box-based thresholds require the original rounding, not accumulated translates. p2.Offset(0, verticalShift); p2.UpdateBounds(); @@ -361,6 +360,8 @@ namespace OpenNest.Engine.Fill var anchor = new Vector(workArea.X - bbox.Left, workArea.Y - bbox.Bottom); part1.Offset(anchor); part2.Offset(anchor); + // Keep these recomputations: translating the cached boxes can differ by an + // ulp, changing exact layouts and the pair-fit/column-tiling thresholds. part1.UpdateBounds(); part2.UpdateBounds(); diff --git a/OpenNest.Engine/Strategies/FillHelpers.cs b/OpenNest.Engine/Strategies/FillHelpers.cs index ea6e76f..71ea34a 100644 --- a/OpenNest.Engine/Strategies/FillHelpers.cs +++ b/OpenNest.Engine/Strategies/FillHelpers.cs @@ -18,6 +18,8 @@ namespace OpenNest.Engine.Strategies foreach (var part in groupParts) { var clone = (Part)part.Clone(); + // Keep the recompute: at angle 0, accumulated source translations can + // leave a different cached box than Program.BoundingBox() + Location. clone.UpdateBounds(); if (!angle.IsEqualTo(0)) diff --git a/OpenNest.Tests/Fill/FillExtentsTests.cs b/OpenNest.Tests/Fill/FillExtentsTests.cs index 02778df..95b36f1 100644 --- a/OpenNest.Tests/Fill/FillExtentsTests.cs +++ b/OpenNest.Tests/Fill/FillExtentsTests.cs @@ -64,6 +64,160 @@ public class FillExtentsTests Assert.Equal(before, Snapshot(drawing)); } + [Theory] + [MemberData(nameof(DifferentialCases))] + public void Fill_Thresholds_MatchPreChangePairFitAndColumnBranches(string shape, double spacing, bool rotated) + { + var drawing = MakeFixture(shape); + var angle = rotated ? System.Math.PI / 6 : 0; + var before = Snapshot(drawing); + var roomy = new Box(3.1, 5.3, 100, 100); + var seed = Invoke(new LegacyFillExtents(roomy, spacing), "BuildPair", drawing, angle); + var bbox = PairBounds(seed); + // The reference BuildPair is identical to pre-Task-3 production. Task 2's + // finite/nonnegative pitch shortcut gives the same column, not different poses. + foreach (var axis in new[] { "length", "width", "column" }) + { + foreach (var fits in new[] { false, true }) + { + var delta = fits ? 1e-9 : -1e-9; + var length = axis == "length" ? bbox.Length - OpenNest.Math.Tolerance.Epsilon + delta : 100; + var width = axis == "width" ? bbox.Width - OpenNest.Math.Tolerance.Epsilon + delta + : axis == "column" ? 2 * bbox.Width + spacing - OpenNest.Math.Tolerance.Epsilon + delta : 100; + var area = new Box(roomy.X, roomy.Y, length, width); + var areaBefore = Bounds(area); + var legacy = new LegacyFillExtents(area, spacing); + var filler = new FillExtents(area, spacing); + var expectedPair = Invoke(legacy, "BuildPair", drawing, angle); + var actualPair = Invoke(filler, "BuildPair", drawing, angle); + Assert.Equal(axis == "column" || fits, expectedPair != null); + Assert.Equal(expectedPair == null, actualPair == null); + if (expectedPair != null) + { + AssertPairEqual(expectedPair, actualPair!); + var expectedColumn = (List)Invoke(legacy, "BuildColumn", expectedPair); + var actualColumn = (List)Invoke(filler, "BuildColumn", actualPair!); + if (axis == "column") + Assert.Equal(fits ? 4 : 2, expectedColumn.Count); + AssertSameLayout(expectedColumn, actualColumn); + } + var expectedProgress = new List>(); + var actualProgress = new List>(); + var expected = legacy.Fill(drawing, angle, reportProgress: (parts, _) => expectedProgress.Add(parts)); + var actual = filler.Fill(drawing, angle, reportProgress: (parts, _) => actualProgress.Add(parts)); + AssertSameLayout(expected, actual); + Assert.Equal(expectedProgress.Count, actualProgress.Count); + for (var i = 0; i < expectedProgress.Count; i++) + AssertSameLayout(expectedProgress[i], actualProgress[i]); + Assert.Equal(areaBefore, Bounds(area)); + Assert.Equal(before, Snapshot(drawing)); + } + } + } + + [Theory] + [MemberData(nameof(DifferentialCases))] + public void Fill_AdjacentDoubleThresholds_PreservePairAndColumnDecisions(string shape, double spacing, bool rotated) + { + var drawing = MakeFixture(shape); + var angle = rotated ? System.Math.PI / 6 : 0; + var roomy = new Box(3.1, 5.3, 100, 100); + var seed = Invoke(new LegacyFillExtents(roomy, spacing), "BuildPair", drawing, angle); + var bbox = PairBounds(seed); + foreach (var axis in new[] { "length", "width", "column" }) + { + var threshold = (axis == "length" ? bbox.Length : axis == "width" ? bbox.Width + : 2 * bbox.Width + spacing) - OpenNest.Math.Tolerance.Epsilon; + var decisions = new HashSet(); + foreach (var size in AdjacentDoubles(threshold)) + { + var area = new Box(roomy.X, roomy.Y, axis == "length" ? size : 100, + axis == "length" ? 100 : size); + var legacy = new LegacyFillExtents(area, spacing); + var filler = new FillExtents(area, spacing); + var expectedPair = Invoke(legacy, "BuildPair", drawing, angle); + var actualPair = Invoke(filler, "BuildPair", drawing, angle); + Assert.Equal(expectedPair == null, actualPair == null); + if (axis != "column") + decisions.Add(expectedPair != null); + if (expectedPair == null) + continue; + AssertPairEqual(expectedPair, actualPair!); + var expected = (List)Invoke(legacy, "BuildColumn", expectedPair); + var actual = (List)Invoke(filler, "BuildColumn", actualPair!); + AssertSameLayout(expected, actual); + if (axis == "column") + decisions.Add(expected.Count == 4); + } + Assert.Contains(false, decisions); + Assert.Contains(true, decisions); + } + } + + [Theory] + [MemberData(nameof(DifferentialCases))] + public void TryShiftDirection_AdjacentWidthThresholds_PreserveAcceptance(string shape, double spacing, bool rotated) + { + var drawing = MakeFixture(shape); + var angle = rotated ? System.Math.PI / 6 : 0; + var area = new Box(3.1, 5.3, 100, 100); + var legacy = new LegacyFillExtents(area, spacing); + var filler = new FillExtents(area, spacing); + var expectedPair = Invoke(legacy, "BuildPair", drawing, angle); + var actualPair = Invoke(filler, "BuildPair", drawing, angle); + foreach (var shift in new[] { -0.3, 0.3 }) + { + // Locate adjacent rejected/accepted doubles using only the frozen pre-change + // path; do not derive the oracle from the implementation under test. + var rejected = 0.0; + var accepted = 100.0; + for (var i = 0; i < 64; i++) + { + var middle = (rejected + accepted) / 2; + if (Invoke(legacy, "TryShiftDirection", expectedPair, shift, middle) == null) + rejected = middle; + else + accepted = middle; + } + Assert.Equal(accepted, System.Math.BitIncrement(rejected)); + foreach (var width in new[] { rejected, accepted }) + { + var expected = Invoke(legacy, "TryShiftDirection", expectedPair, shift, width); + var actual = Invoke(filler, "TryShiftDirection", actualPair, shift, width); + Assert.Equal(width == accepted, expected != null); + Assert.Equal(expected == null, actual == null); + if (expected != null) + AssertPairEqual(expected, actual!); + } + } + } + + private static IEnumerable AdjacentDoubles(double value) + { + yield return value; + var lower = value; + var upper = value; + for (var i = 0; i < 4; i++) + { + lower = System.Math.BitDecrement(lower); + upper = System.Math.BitIncrement(upper); + yield return lower; + yield return upper; + } + } + + private static Box PairBounds(object pair) => (Box)pair.GetType().GetProperty("Bbox")!.GetValue(pair)!; + + private static void AssertPairEqual(object expected, object actual) + { + Assert.Equal(Bounds(PairBounds(expected)), Bounds(PairBounds(actual))); + var expectedParts = new[] { "Part1", "Part2" }.Select(name => + (Part)expected.GetType().GetProperty(name)!.GetValue(expected)!).ToList(); + var actualParts = new[] { "Part1", "Part2" }.Select(name => + (Part)actual.GetType().GetProperty(name)!.GetValue(actual)!).ToList(); + AssertSameLayout(expectedParts, actualParts); + } + [Theory] [InlineData(5, 5)] [InlineData(15, 10)] // One rectangle fits, the pair does not. @@ -185,6 +339,35 @@ public class FillExtentsTests } #if DEBUG + [Theory] + [InlineData(0.0)] + [InlineData(0.5)] + public void Fill_BoundsWork_PerformsFewerUpdates(double spacing) + { + var area = new Box(3, 5, 45, 27); + var drawing = MakeFixture("triangle"); + PerfCounters.Reset(); + try + { + // Task 2 removed boundary preparation, not Part.UpdateBounds calls, so + // this frozen baseline has the pre-Task-3 full-fill bounds-update count. + var expected = new LegacyFillExtents(area, spacing).Fill(drawing); + var before = PerfCounters.PartBoundsUpdates; + PerfCounters.Reset(); + var actual = new FillExtents(area, spacing).Fill(drawing); + var after = PerfCounters.PartBoundsUpdates; + output.WriteLine($"bounds spacing={spacing}: before={before}, actual={after}, parts={actual.Count}"); + AssertSameLayout(expected, actual); + Assert.Equal(13, before); + Assert.True(after < before, $"Expected fewer than {before} part bounds updates, actual {after}."); + Assert.Equal(10, after); + } + finally + { + PerfCounters.Reset(); + } + } + [Theory] [InlineData(0.0)] [InlineData(0.5)] @@ -284,7 +467,8 @@ public class FillExtentsTests for (var i = 0; i < expected.Count; i++) { Assert.Same(expected[i].BaseDrawing, actual[i].BaseDrawing); - Assert.Equal(expected[i].Location, actual[i].Location); + Assert.Equal((expected[i].Location.X, expected[i].Location.Y), + (actual[i].Location.X, actual[i].Location.Y)); Assert.Equal(expected[i].Rotation, actual[i].Rotation); Assert.Equal(Bounds(expected[i].BoundingBox), Bounds(actual[i].BoundingBox)); Assert.Equal(ProgramValues(expected[i].Program), ProgramValues(actual[i].Program)); @@ -315,12 +499,12 @@ public class FillExtentsTests { values.Add(code.GetType()); if (code is Motion motion) - values.Add(motion.EndPoint); + values.Add((motion.EndPoint.X, motion.EndPoint.Y)); if (code is LinearMove line) values.Add(line.Layer); if (code is ArcMove arc) { - values.Add(arc.CenterPoint); + values.Add((arc.CenterPoint.X, arc.CenterPoint.Y)); values.Add(arc.Rotation); values.Add(arc.Layer); } diff --git a/OpenNest.Tests/Fill/FillPerformanceTests.cs b/OpenNest.Tests/Fill/FillPerformanceTests.cs index d3ca042..2a27790 100644 --- a/OpenNest.Tests/Fill/FillPerformanceTests.cs +++ b/OpenNest.Tests/Fill/FillPerformanceTests.cs @@ -139,6 +139,66 @@ public class FillPerformanceTests ReportGroupSummary("custom", customSamples, callsPerBatch); } + [SkippableFact] + public void RotatedPattern_ReportsBoundsConstruction() + { + Skip.IfNot(Environment.GetEnvironmentVariable("OPENNEST_RUN_FILL_PERF") == "1", + "Set OPENNEST_RUN_FILL_PERF=1 to run opt-in fill microbenchmarks."); + var drawing = FillExtentsTests.MakeFixture("arc"); + var group = Enumerable.Range(0, 32).Select(i => + new Part(drawing, new Vector(11.25 + i % 8 * 12, 13.5 + i / 8 * 10))).ToList(); + var angles = new[] { 0.0, 0.37 }; + var builds = angles.Select(angle => new Func>(() => + FillHelpers.BuildRotatedPattern(group, angle).Parts)).ToArray(); + var expected = angles.Select(angle => + OpenNest.Tests.Strategies.FillHelpersTests.PreChangeRotatedPattern(group, angle).Parts).ToArray(); + var warmupCalls = 1_000; + var callsPerBatch = 5_000; + var repetitions = 7; +#if DEBUG + output.WriteLine("Configuration=Debug (diagnostic only; use Release for measurements)."); +#else + output.WriteLine("Configuration=Release."); +#endif + output.WriteLine($"Runtime={RuntimeInformation.FrameworkDescription}; OS={RuntimeInformation.OSDescription}; " + + $"architecture={RuntimeInformation.ProcessArchitecture}; processors={Environment.ProcessorCount}; " + + $"Stopwatch.Frequency={Stopwatch.Frequency} ticks/s."); + output.WriteLine($"rotated-pattern: 32 native-arc parts on an 8-column 12x10 grid from (11.25,13.5); " + + $"angle=0 or 0.37; warmup=2 x {warmupCalls}; measured={repetitions} x {callsPerBatch}; " + + "angle batch order alternates. Real synchronous production construction only; setup, reference, " + + "assertions/output excluded; clone/rotation/aggregate bounds, GC and count consumption included. " + + "Warm drawing/JIT; no forced GC/cache reset. Current-thread allocations, not RSS or a whole-job benchmark."); + for (var mode = 0; mode < builds.Length; mode++) + FillExtentsTests.AssertSameLayout(expected[mode], builds[mode]()); + for (var batch = 0; batch < 2; batch++) + for (var slot = 0; slot < builds.Length; slot++) + MeasureExtents(builds[(slot + batch) % builds.Length], warmupCalls); + var samples = angles.Select(_ => new ExtentsSample[repetitions]).ToArray(); + for (var batch = 0; batch < repetitions; batch++) + { + for (var slot = 0; slot < builds.Length; slot++) + { + var mode = (slot + batch) % builds.Length; + samples[mode][batch] = MeasureExtents(builds[mode], callsPerBatch); + } + for (var mode = 0; mode < builds.Length; mode++) + { + var sample = samples[mode][batch]; + Assert.Equal((long)callsPerBatch * group.Count, sample.PartCount); + FillExtentsTests.AssertSameLayout(expected[mode], sample.LastResult); + output.WriteLine(FormattableString.Invariant( + $"rotated-pattern angle={angles[mode]} batch={batch + 1}: ms={sample.Milliseconds:F6}; bytes={sample.AllocatedBytes}; parts={sample.PartCount}.")); + } + } + for (var mode = 0; mode < builds.Length; mode++) + { + var times = samples[mode].Select(s => s.Milliseconds).OrderBy(t => t).ToArray(); + var bytes = samples[mode].Select(s => s.AllocatedBytes).OrderBy(b => b).ToArray(); + output.WriteLine(FormattableString.Invariant( + $"rotated-pattern angle={angles[mode]}: batch ms min/median/max={times[0]:F6}/{times[repetitions / 2]:F6}/{times[^1]:F6}; us/call min/median/max={times[0] * 1000 / callsPerBatch:F3}/{times[repetitions / 2] * 1000 / callsPerBatch:F3}/{times[^1] * 1000 / callsPerBatch:F3}; batch bytes min/median/max={bytes[0]}/{bytes[repetitions / 2]}/{bytes[^1]}; B/call={(double)bytes[repetitions / 2] / callsPerBatch:F3}.")); + } + } + [SkippableFact] public void Extents_ReportsRepeatedColumnRebuilds() { diff --git a/OpenNest.Tests/Strategies/FillHelpersTests.cs b/OpenNest.Tests/Strategies/FillHelpersTests.cs index f096e1a..670992c 100644 --- a/OpenNest.Tests/Strategies/FillHelpersTests.cs +++ b/OpenNest.Tests/Strategies/FillHelpersTests.cs @@ -11,6 +11,86 @@ namespace OpenNest.Tests.Strategies; [Collection(nameof(FillCacheCollection))] public class FillHelpersTests { + [Theory] + [InlineData("accumulated-translation", 0.0)] + [InlineData("accumulated-translation", 0.37)] + [InlineData("translated", 0.0)] + [InlineData("translated", 0.37)] + [InlineData("pre-rotated", 0.0)] + [InlineData("pre-rotated", 0.37)] + [InlineData("canonical", 0.0)] + [InlineData("canonical", 0.37)] + [InlineData("arc", 0.0)] + [InlineData("arc", 0.37)] + [InlineData("nonzero-program-origin", 0.0)] + [InlineData("nonzero-program-origin", 0.37)] + public void BuildRotatedPattern_MatchesPreChangePosesBoundsAndOwnership(string scenario, double angle) + { + var drawing = OpenNest.Tests.Fill.FillExtentsTests.MakeFixture(scenario == "arc" ? "arc" : "concave"); + if (scenario is "pre-rotated" or "canonical") + drawing.Program.Rotate(0.6); + double canonicalAngle = 0; + if (scenario == "canonical") + { + // Program.Rotate mutates in place; refresh the canonical angle the same way + // a rotated CAD import carries it, or AsCanonicalCopy takes the zero-angle path. + drawing.RecomputeCanonicalAngle(); + canonicalAngle = drawing.Source.Angle; + Assert.False(OpenNest.Math.Tolerance.IsEqualTo(canonicalAngle, 0)); + drawing = CanonicalFrame.AsCanonicalCopy(drawing); + Assert.Equal(0.0, drawing.Source.Angle); + } + if (scenario == "accumulated-translation") + drawing.Program.Offset(new Vector(0.1, 0.1)); + if (scenario == "nonzero-program-origin") + drawing.Program.Offset(new Vector(0.125, -0.375)); + var group = new List + { + new(drawing, new Vector(11.25, 13.5)), + new(drawing, new Vector(31.75, 27.25)), + }; + if (scenario == "translated") + foreach (var part in group) + part.Offset(3.5, -2.25); + if (scenario == "accumulated-translation") + { + group = new List { new(drawing, new Vector(0.1, 0.1)) }; + group[0].Offset(1.1, 1.1); + } + var before = Snapshot(group); + var expected = PreChangeRotatedPattern(group, angle); + var actual = FillHelpers.BuildRotatedPattern(group, angle); + OpenNest.Tests.Fill.FillExtentsTests.AssertSameLayout(expected.Parts, actual.Parts); + Assert.Equal(Bounds(expected.BoundingBox), Bounds(actual.BoundingBox)); + AssertNoInputPartsReturned(group, actual.Parts); + Assert.Equal(before, Snapshot(group)); + for (var i = 0; i < group.Count; i++) + { + Assert.NotSame(group[i].Program, actual.Parts[i].Program); + // Clone must not reapply the baked drawing rotation before adding the angle. + // Normalize both sides: a baked canonical rotation can already sit at exactly 2pi. + Assert.Equal(OpenNest.Math.Angle.NormalizeRad(group[i].Rotation + angle), + OpenNest.Math.Angle.NormalizeRad(actual.Parts[i].Rotation)); + } + } + + // The exact pre-Task-3 helper, kept local to characterization, never used for timings. + internal static Pattern PreChangeRotatedPattern(List group, double angle) + { + var pattern = new Pattern(); + var center = ((IEnumerable)group).GetBoundingBox().Center; + foreach (var part in group) + { + var clone = (Part)part.Clone(); + clone.UpdateBounds(); + if (!OpenNest.Math.Tolerance.IsEqualTo(angle, 0)) + clone.Rotate(angle, center); + pattern.Parts.Add(clone); + } + pattern.UpdateBounds(); + return pattern; + } + [Theory] [InlineData(5, 9, 8, 7)] [InlineData(10, 4, 7, 8)] @@ -185,6 +265,30 @@ public class FillHelpersTests } #if DEBUG + [Theory] + [InlineData(0.0, 2)] + [InlineData(0.37, 4)] + public void BuildRotatedPattern_BoundsWork_RetainsRequiredCloneRecomputation(double angle, long expectedUpdates) + { + var group = MakeGroup(); + var expected = PreChangeRotatedPattern(group, angle); + PerfCounters.Reset(); + try + { + var pattern = FillHelpers.BuildRotatedPattern(group, angle); + // Removing the clone recompute changes accumulated-translation boxes at angle 0. + // This site was retained, not optimized. Pattern.UpdateBounds aggregates boxes; + // it does not call Part.UpdateBounds and remains outside this counter. + Assert.Equal(expectedUpdates, PerfCounters.PartBoundsUpdates); + AssertSameLayout(expected.Parts, pattern.Parts); + Assert.Equal(Bounds(expected.BoundingBox), Bounds(pattern.BoundingBox)); + } + finally + { + PerfCounters.Reset(); + } + } + [Theory] [InlineData(5, 9, 0, 2)] [InlineData(5, 9, 1, 0)] @@ -231,7 +335,8 @@ public class FillHelpersTests for (var i = 0; i < expected.Count; i++) { Assert.Same(expected[i].BaseDrawing, actual[i].BaseDrawing); - Assert.Equal(expected[i].Location, actual[i].Location); + Assert.Equal((expected[i].Location.X, expected[i].Location.Y), + (actual[i].Location.X, actual[i].Location.Y)); Assert.Equal(expected[i].Rotation, actual[i].Rotation); Assert.Equal(Bounds(expected[i].BoundingBox), Bounds(actual[i].BoundingBox)); } @@ -270,7 +375,7 @@ public class FillHelpersTests values.Add(part); values.Add(part.BaseDrawing); values.Add(part.BaseDrawing.Area); - values.Add(part.Location); + values.Add((part.Location.X, part.Location.Y)); values.Add(part.Rotation); values.Add(Bounds(part.BoundingBox)); foreach (var program in new[] { part.Program, part.BaseDrawing.Program }) @@ -282,7 +387,13 @@ public class FillHelpersTests { values.Add(code); if (code is Motion motion) - values.Add(motion.EndPoint); + values.Add((motion.EndPoint.X, motion.EndPoint.Y)); + if (code is ArcMove arc) + { + values.Add((arc.CenterPoint.X, arc.CenterPoint.Y)); + values.Add(arc.Rotation); + values.Add(arc.Layer); + } } } } diff --git a/README.md b/README.md index a397639..4a553ac 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,8 @@ The group-pattern measurement also compares default scoring with a custom compar The extents measurement exercises repeated column rebuilding with a closed triangle at zero and positive spacing. It times the synchronous production fill and reports current-thread allocations; the frozen legacy implementation is used only for correctness checks outside timing. Run only that case with `--filter "FullyQualifiedName~Extents_ReportsRepeatedColumnRebuilds"`. Extents behavior, overlap-fallback, and Debug boundary-preparation checks use `--filter "FullyQualifiedName~FillExtentsTests|FullyQualifiedName~StrategyOverlapTests"`. Finite nonnegative spacing uses the equivalent bounding-box pitch without discarded vertical boundary preparation; negative/nonfinite spacing retains the legacy calculation rather than adding validation. +The synchronous rotated-pattern construction control uses a 32-part native-arc group at angles 0 and 0.37. Run it with `--filter "FullyQualifiedName~RotatedPattern_ReportsBoundsConstruction"`. Task 3 reuses the extents benchmark above; Debug `--filter "FullyQualifiedName~BoundsWork"` checks part-bounds work. Only three extents recomputations were removable: anchor, vertical-shift, and group-clone recomputations remain because removing them changes exact floating-point layouts. See the measured report for the partial-delivery evidence and inconclusive timing results. + ### Quick start 1. File > New Nest diff --git a/docs/performance/fill-performance.md b/docs/performance/fill-performance.md index 4ecc848..8ec6a90 100644 --- a/docs/performance/fill-performance.md +++ b/docs/performance/fill-performance.md @@ -365,3 +365,107 @@ dotnet test OpenNest.Tests/OpenNest.Tests.csproj -c Debug \ For an independent before/after comparison, keep the delivered tests/diagnostics identical in two disposable trees and use the baseline `FillExtents.cs` only in the before tree. Rebuild each configuration and verify the source/harness hashes. Do not change production spacing policy to reproduce invalid-domain cases or substitute the frozen test type for the measured production method. Final logs, TRX files, source restoration snapshot, manifests, and parsed rows were held in `/home/aj/.hermes/cache/scratch/opennest-task2-resume-20260925/` through review. The report preserves all final timing rows, allocation totals, source hashes, runtime and test summaries; temporary evidence and the superseded resumed exploration directory are removed before commit. Next planned hardening is the separately reviewable redundant-bounds slice with threshold-fit and canonical-frame safety, not a geometry repair bundled into this change. + +## Task 3 — remove provably redundant bounds walks — 2026-09-25 + +### Scope and per-site decisions + +`Part.Offset` translates the cached `BoundingBox` arithmetically and `Part.Clone` copies it exactly, so several `UpdateBounds()` calls in the extents/pattern paths recompute a box that just changed by pure translation. Recomputation is mathematically equivalent but not bitwise identical to accumulated translates, and the extents pipeline compares boxes at `Tolerance.Epsilon` fit thresholds. Each candidate removal therefore had to keep every characterized layout, box, and threshold branch bitwise identical (the differential harness compares `(X, Y, Length, Width)` and location tuples with exact `double` equality, not tolerance — `Vector.Equals` itself is tolerance-based, so location/coordinate snapshots were switched to scalar tuples to expose ulp deltas). + +| Site | Decision | Evidence | +| --- | --- | --- | +| `BuildPair` after `Offset(startOffset, 0)` | Removed | All characterization green; slide uses locations, not cached bounds. | +| `BuildPair` after `Offset(-dist, 0)` | Removed | All characterization green. | +| `TryShiftDirection` after `Offset(partSpacing, 0)` | Removed | The retained post-vertical-shift recompute covers both offsets. | +| `TryShiftDirection` after `Offset(0, verticalShift)` | Retained | Rotated triangle spacing 0.5: box Y `5.6025660004937174` → `5.6025660004937183`. `Compactor.Push` box thresholds consume this box. | +| `AnchorToWorkArea` part1/part2 recompute | Retained | Part2-only removal changed rectangle differential X `13.499999999999998` → `13.5` and arc pair height `8.0000000000000036` → `8`; part1 removal changed X `3.1000000000000005` → `3.100000000000001`. Part1-only removal failed 6 of 101 characterization cases; part2-only removal failed 25 of 101, including overlap fallback. The anchor `workArea.X - bbox.Left` round-trip lands a ulp away from `X += anchor`. | +| `BuildRotatedPattern` after `Clone()` | Retained | Accumulated-translation input at angle 0: box `1.3000000000000003` → `1.3`. Source comments now record why each retained site stays. | + +No change to `Part.Offset`, `Part.Clone`, `Part.Rotate`, `Part.UpdateBounds`, `PairBbox`, `pattern.UpdateBounds()`, the Task 2 pitch guard, or `LegacyFillExtents.cs` (frozen SHA-256 `b50b3b64014446d9688facc0b711b59cf126b64037fdf667603ce3d85859a324` unchanged). `PerfCounters.PartBoundsUpdates` (Debug-only, incremented at the top of `Part.UpdateBounds`) was added per the plan. + +### Source and measurement provenance + +- Base: `4ec92c95ec10c5c877e9fb0f34f197d7e23b87b0` on `master`. Final implementation is delivered with this report; resolve with `git log -1 --format=%H -- OpenNest.Engine/Fill/FillExtents.cs`. +- `FillExtents.cs` SHA-256 before `efaacfca83a6d94bb7fdc65ba4da4be09c97c14c9c4e161491d48d64a69d1be3` (Task 2 delivered) → after `13be137690ad5f60e0fe3874ec0cb0b93e9864b97d7eee7f3aa65c9b962053a4`. +- `FillHelpers.cs` before `a1c8a1bc155dbe62439f8345a3a54e06147a5404a6a46835a30b66d52cceccfa` → after `0a35075a573c7f17f9c8095a5a7bd56d0a4ea935ec1c9d8da2c4c9d78e0a9af0` (comment-only; site retained). +- Identical before/after harness `FillPerformanceTests.cs` SHA-256 `93ec06c5b59aecfe5cb9311380afea507cc134e77df7535036fb451ab0f63e29`. +- Before timings ran the actual pre-change production files (restored temporarily, restored back in `finally`); after timings the delivered files. Same machine, serial Release: `hermes`, Ubuntu 24.04.5 LTS x64, KVM, four vCPUs presented as AMD Ryzen 9 5900X, SDK 10.0.112, .NET 8.0.31, 1 GHz stopwatch. Shared VM, no pinning. + +### Work-removal evidence (genuine red/green) + +New Debug counter tests (serialized in `FillCacheCollection`, reset in `finally`) failed first on the unmodified production code: `Fill_BoundsWork_PerformsFewerUpdates` expected fewer than 13 part-bounds updates, actual 13 (both spacings); the group-construction work assertion expected 0/2 updates at angles 0/0.37, actual 2/4. After the change: full triangle fill 13 → 10 updates with 24 bitwise-identical parts (both spacings). The group assertions were rewritten to pin the retained counts (2 at angle 0, 4 at 0.37 = retained clone recompute + rotate recompute per part) because removing the clone recompute failed behavior characterization — disclosed partial delivery, not an optimized site. `Pattern.UpdateBounds` aggregates boxes and does not call `Part.UpdateBounds`, so it is outside this counter. Per fill the three removals save three `UpdateBounds` calls (three `Box` allocations — 144 bytes) reproduced exactly in every measured batch. + +### Characterization added (all passing pre-change, 133 targeted Release) + +- 16 extents threshold cases: pair-fit, column-tiling, and column-count branches on both sides of `Tolerance.Epsilon`, nonzero work-area origin, progress layouts, input snapshots. +- 16 adjacent-double pair/column cases (`BitIncrement`/`BitDecrement` neighborhoods with asserted both-side decisions) and 16 adjacent-double `TryShiftDirection` width-acceptance cases (binary-searched rejected/accepted doubles located using only the frozen pre-change path, ±shifts). +- 12 `BuildRotatedPattern` cases: translated, accumulated-translation, pre-rotated, canonical-frame, native-arc, and nonzero-program-origin groups at angles 0 and 0.37, checking exact poses/bounds against a local pre-change reference, input non-mutation, clone program ownership, and no double rotation of pre-rotated inputs. +- Frozen-legacy differentials, threshold branch equality, and the retained-site RED logs (`task3-site-*.log`) reconcile the per-site table above. + +### Tests (parent independently reran the full suites on the final tree) + +| Suite | Passed | Skipped | Failed | +| --- | ---: | ---: | ---: | +| Targeted Release (`FillExtentsTests|FillHelpersTests|CanonicalFrameTests|StripeFillerTests`) | 133 | 0 | 0 | +| AGENTS.md Debug work filter | 137 | 0 | 0 | +| Full main Release | 1266 | 16 | 0 | +| Full main Debug | 1283 | 16 | 0 | +| Full engine Release | 300 | 0 | 0 | + +Skips are the 12 optional CHR-font fixtures plus four opt-in benchmarks (the new rotated-pattern case adds one). Gate verified: unset and `0` skip all category tests. `.NET 8.0.31` adapters confirmed. Changed-file `dotnet format whitespace --verify-no-changes` and `git diff --check` passed; no new production warnings. + +### Reviews + +Implementation, spec review, and quality/integration review ran as three independent agent sessions (the earlier same-agent serial constraint was lifted by the user for this session). Spec review initially found one Important issue — the canonical fixture consumed a stale `Source.Angle` and duplicated the pre-rotated scenario — plus one Minor report-wording error; both were fixed (fixture now refreshes the canonical angle and asserts a nonzero source-to-canonical transformation; the failure counts are stated as 6/101 and 25/101), and the strengthened fixture additionally surfaced a benign test-assertion normalization issue at a baked 2π rotation, fixed by normalizing both sides. Subsequent independent quality/integration review returned PASS with no Critical, Important, or Minor findings: production diff traced through slide/Compactor consumers, counter isolation, adjacent-double oracle independence, hash/median/allocation reconciliation against raw logs, integration sweep across fill-strategy and jobs callers, and disclosure honesty were all verified by that reviewer. Pre-existing DXF-fixture-dependent tests still return early without their Windows fixture; no Windows UI runtime test or ONNX inference was run. + +### Results + +All before/after timing ranges overlap: elapsed-time improvement is inconclusive, consistent with three saved `Program.BoundingBox()` walks being small against full fills. Allocation savings are exact and reproduce in every batch: extents spacing 0 `839,624` → `839,480` bytes/fill and spacing 0.5 `1,028,544` → `1,028,400` bytes/fill (−144 B/fill). Rotated-pattern construction is an unchanged-code control: identical allocations per call at both angles. + +Extents: milliseconds per 200 production fills, seven batches each, alternating spacing order, warmup excluded. Rotated-pattern: milliseconds per 5,000 constructions of a 32-part native-arc group. + +| Workload | Batch | Before ms | After ms | +| --- | ---: | ---: | ---: | +| extents spacing 0 | 1 | 461.646777 | 502.378600 | +| extents spacing 0 | 2 | 439.442203 | 446.609639 | +| extents spacing 0 | 3 | 450.272109 | 457.012036 | +| extents spacing 0 | 4 | 433.095759 | 454.402604 | +| extents spacing 0 | 5 | 443.489319 | 447.382522 | +| extents spacing 0 | 6 | 433.504166 | 448.477156 | +| extents spacing 0 | 7 | 441.588374 | 439.879594 | +| extents spacing 0.5 | 1 | 460.691045 | 439.973796 | +| extents spacing 0.5 | 2 | 457.895526 | 443.225746 | +| extents spacing 0.5 | 3 | 444.315207 | 451.898487 | +| extents spacing 0.5 | 4 | 436.733945 | 447.851680 | +| extents spacing 0.5 | 5 | 451.266260 | 431.428815 | +| extents spacing 0.5 | 6 | 438.338321 | 455.657180 | +| extents spacing 0.5 | 7 | 448.995034 | 438.982789 | +| rotated-pattern angle 0 | 1 | 92.121779 | 95.085965 | +| rotated-pattern angle 0 | 2 | 85.115166 | 83.764479 | +| rotated-pattern angle 0 | 3 | 84.692335 | 82.859009 | +| rotated-pattern angle 0 | 4 | 90.233547 | 85.003329 | +| rotated-pattern angle 0 | 5 | 85.081651 | 83.281808 | +| rotated-pattern angle 0 | 6 | 78.809908 | 77.355562 | +| rotated-pattern angle 0 | 7 | 78.280977 | 80.372130 | +| rotated-pattern angle 0.37 | 1 | 184.337007 | 186.875744 | +| rotated-pattern angle 0.37 | 2 | 170.493990 | 155.509393 | +| rotated-pattern angle 0.37 | 3 | 153.992846 | 155.681698 | +| rotated-pattern angle 0.37 | 4 | 155.207434 | 156.663232 | +| rotated-pattern angle 0.37 | 5 | 158.498327 | 149.814054 | +| rotated-pattern angle 0.37 | 6 | 157.148604 | 150.090405 | +| rotated-pattern angle 0.37 | 7 | 150.252399 | 150.547539 | + +Medians: extents spacing 0 2207.942 → 2242.386 µs/call, spacing 0.5 2244.975 → 2216.129 µs/call; rotated-pattern angle 0 17.016 → 16.656 µs/call, angle 0.37 31.430 → 31.102 µs/call. Every batch produced the expected part counts (extents 4,800; rotated-pattern 160,000) and layout assertions ran on the timed path's output outside timing. Current-thread synchronous allocations only; no whole-job or RSS claims. + +### Reproduction and limitations + +```bash +OPENNEST_RUN_FILL_PERF=1 dotnet test OpenNest.Tests/OpenNest.Tests.csproj -c Release \ + --filter 'Category=FillPerformance' --logger 'console;verbosity=detailed' +dotnet test OpenNest.Tests/OpenNest.Tests.csproj -c Release \ + --filter 'FullyQualifiedName~FillExtentsTests|FullyQualifiedName~FillHelpersTests|FullyQualifiedName~CanonicalFrameTests|FullyQualifiedName~StripeFillerTests' +dotnet test OpenNest.Tests/OpenNest.Tests.csproj -c Debug \ + --filter 'FullyQualifiedName~DefaultFillComparerWorkTests|FullyQualifiedName~FillHelpersTests|FullyQualifiedName~FillExtentsTests|FullyQualifiedName~StrategyOverlapTests' +``` + +This is a partial delivery by design: three of six candidate recomputations were removable; the three retained sites are documented with their observed ulp deltas, and their `UpdateBounds` calls must not be removed without re-running the threshold and overlap-fallback characterization. Timing is inconclusive; only allocation and work-counter removal are demonstrated. No representative production corpus, Windows UI runtime test, or ONNX inference was run. Task 4 and gated follow-ups are not included.