perf: Fix LIS algorithm bug causing 999 DOM moves instead of 2#59
perf: Fix LIS algorithm bug causing 999 DOM moves instead of 2#59MCGPPeters merged 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a performance regression in keyed DOM reordering by correcting the LIS computation (reducing unnecessary DOM moves), and adds supporting perf/memory improvements and tests.
Changes:
- Fix LIS marking logic used by keyed child diffing and add unit tests for swap/reorder scenarios.
- Reduce allocations on hot paths via ArrayPool/List pooling in DOM operations.
- Adjust CI benchmark comparison threshold and improve browser E2E setup (Playwright cache); defer OTel CDN init in bundled JS to improve startup.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/compare-benchmarks.py | Updates default throughput regression threshold and documents CI variance rationale. |
| Abies/wwwroot/abies.js | Defers OTel CDN initialization and adds a local shim with verbosity cache reset support. |
| Abies/DOM/Operations.cs | Fixes LIS usage in keyed diffing and adds pooling for patch serialization and LIS membership tracking. |
| Abies.Tests/DomBehaviorTests.cs | Adds tests validating minimized MoveChild patches for swap/reorder scenarios. |
| Abies.SubscriptionsDemo/wwwroot/abies.js | Same deferred OTel initialization + shim changes as main Abies JS. |
| Abies.Presentation/wwwroot/abies.js | Same deferred OTel initialization + shim changes as main Abies JS. |
| .github/workflows/e2e.yml | Caches Playwright browsers to reduce CI time and flakiness. |
| .github/workflows/benchmark.yml | Uses the updated throughput threshold when comparing benchmarks. |
| .github/instructions/memory.instructions.md | Documents LIS fix + benchmark findings; adds note about dotnet format multi-targeting pitfalls. |
| // Rent a bool array instead of allocating HashSet<int> | ||
| var inLIS = ArrayPool<bool>.Shared.Rent(newLength); | ||
| try | ||
| { |
There was a problem hiding this comment.
The rented inLIS array may contain stale true values from previous ArrayPool users, but it’s not cleared before calling ComputeLISInto. That can incorrectly skip required moves (because !inLIS[i] may be false even when the position is not in the current LIS). Clear inLIS[0..newLength) immediately after renting (or have ComputeLISInto clear the output span before marking).
| { | |
| { | |
| // Ensure the rented buffer is clean before marking LIS positions | |
| inLIS.AsSpan(0, newLength).Clear(); |
| // The indices returned are positions in oldIndices that form the LIS | ||
| // We mark those positions as "in LIS" (don't need moving) | ||
| ComputeLISInto(oldIndices.AsSpan(0, newLength), inLIS.AsSpan(0, newLength)); |
There was a problem hiding this comment.
The rented inLIS array may contain stale true values from previous ArrayPool users, but it’s not cleared before calling ComputeLISInto. That can incorrectly skip required moves (because !inLIS[i] may be false even when the position is not in the current LIS). Clear inLIS[0..newLength) immediately after renting (or have ComputeLISInto clear the output span before marking).
| for (int i = newLength - 1; i >= 0; i--) | ||
| { | ||
| if (!inLIS.Contains(i)) | ||
| if (!inLIS[i]) |
There was a problem hiding this comment.
The rented inLIS array may contain stale true values from previous ArrayPool users, but it’s not cleared before calling ComputeLISInto. That can incorrectly skip required moves (because !inLIS[i] may be false even when the position is not in the current LIS). Clear inLIS[0..newLength) immediately after renting (or have ComputeLISInto clear the output span before marking).
| // Helper to determine if a URL should be ignored for tracing | ||
| function shouldIgnoreFetchForTracing(url) { | ||
| try { | ||
| if (!url) return false; | ||
| // Ignore OTLP proxy endpoint | ||
| if (/\/otlp\/v1\/traces$/.test(url)) return true; | ||
| // Ignore common collector endpoints like http://localhost:4318/v1/traces | ||
| if (/\/v1\/traces$/.test(url)) return true; | ||
| // Ignore explicitly configured exporter URL if provided | ||
| if (typeof window !== 'undefined' && window.__OTEL_EXPORTER_URL) { | ||
| const configured = String(window.__OTEL_EXPORTER_URL); | ||
| if (configured && url.startsWith(configured)) return true; | ||
| } | ||
| // Ignore Blazor framework/runtime/resource downloads | ||
| if (url.includes('/_framework/')) return true; | ||
| } catch { | ||
| // On any error, fall back to tracing (do not silently skip) | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
shouldIgnoreFetchForTracing doesn’t explicitly ignore the shim’s resolved endpoint value (from meta/global/default) unless it happens to match the hard-coded /v1/traces$ patterns. If a custom exporter URL doesn’t end with /v1/traces, the shim will instrument its own export fetch, which can spiral into recursive span exporting. Add an explicit check to ignore the actual endpoint (and ideally its normalized URL form), and apply the same fix to the duplicated copies in Abies.SubscriptionsDemo/wwwroot/abies.js and Abies.Presentation/wwwroot/abies.js.
| window.fetch = async function(input, init) { | ||
| const url = (typeof input === 'string') ? input : input.url; | ||
| if (shouldIgnoreFetchForTracing(url)) return origFetch(input, init); |
There was a problem hiding this comment.
shouldIgnoreFetchForTracing doesn’t explicitly ignore the shim’s resolved endpoint value (from meta/global/default) unless it happens to match the hard-coded /v1/traces$ patterns. If a custom exporter URL doesn’t end with /v1/traces, the shim will instrument its own export fetch, which can spiral into recursive span exporting. Add an explicit check to ignore the actual endpoint (and ideally its normalized URL form), and apply the same fix to the duplicated copies in Abies.SubscriptionsDemo/wwwroot/abies.js and Abies.Presentation/wwwroot/abies.js.
| private static void ReturnPatchDataList(List<PatchData> list) | ||
| { | ||
| if (list.Count < 1000) // Prevent memory bloat | ||
| { |
There was a problem hiding this comment.
When returning a List<PatchData> to the pool, the list isn’t cleared first, so it can retain references to many PatchData instances (and their referenced objects) for the lifetime of the pooled list. Clearing before enqueueing releases those references earlier and reduces memory retention if the pooled list isn’t rented again soon.
| { | |
| { | |
| list.Clear(); |
| | Benchmark | Abies | Blazor | VanillaJS | | ||
| |-----------|-------|--------|-----------| | ||
| | 05_swap1k | 406.7ms | 94.4ms | 32.2ms | | ||
| ```` |
There was a problem hiding this comment.
There’s a malformed Markdown code fence using four backticks (````), which is likely to break rendering for the remainder of the document. Replace it with a matching triple-backtick fence and ensure it properly closes the intended code block.
| ```` |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rendering Engine Throughput'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.
| Benchmark suite | Current: 08ddfc6 | Previous: d80f18a | Ratio |
|---|---|---|---|
Abies.Benchmarks.Handlers/Create50Handlers |
2556.069035393851 ns (± 32.28510677946083) |
2420.239361572266 ns (± 38.40334347791619) |
1.06 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @MCGPPeters
MCGPPeters
changed the title
The ComputeLISInto function had a critical bug where it tracked 'k' as 'length-1' but the first element never incremented k, causing the binary search to always use hi=0 and produce an LIS of length 1. For the js-framework-benchmark swap test (swap 2 elements in 1000), this caused 999 MoveChild patches instead of 2. Fix: Changed from tracking k (length-1) to lisLen (actual length), with proper increment when lo == lisLen. Benchmark results: - Swap median: 326.6ms → 121.6ms (2.7x faster) - Script time: ~300ms → 96.7ms (3x faster) - DOM moves: 999 → 2 (99.8% reduction) Now only 1.29x slower than Blazor (was 3.47x).
MCGPPeters
force-pushed
the
fix/lis-algorithm-swap-performance
branch
from
98818c3 to
08ddfc6
Compare
📝 Description
What
Fix a critical bug in the LIS (Longest Increasing Subsequence) algorithm that was causing massive performance degradation in the swap benchmark.
Why
The
ComputeLISIntofunction had a bug where it trackedkas "length-1" but the first element never incrementedk, causing the binary search to always usehi=0and produce an LIS of length 1 instead of the correct length.For the js-framework-benchmark swap test (swap 2 elements in 1000), this caused 999 MoveChild patches instead of 2.
How
Changed from tracking
k(length-1) tolisLen(actual length), with proper increment whenlo == lisLen.🔗 Related Issues
Fixes the swap benchmark performance regression identified in performance profiling.
✅ Type of Change
🧪 Testing
Test Coverage
Testing Details
✨ Changes Made
Algorithm Fix
Benchmark Results
Comparison with Blazor
🔍 Code Review Checklist
dotnet format --verify-no-changespasses