fix: address Copilot review comments for ArrayPool clearing #60
Conversation
- Clear inLIS array after renting from ArrayPool to avoid stale data - Clear List<PatchData> before returning to pool to release references - Remove redundant Clear() calls (clear on rent vs on return) - Fix stray markdown fence in memory.instructions.md
There was a problem hiding this comment.
Pull request overview
This PR follows up on earlier performance/pooling work in Operations.cs by improving memory safety when reusing pooled arrays/lists during DOM diffing and batch patch application.
Changes:
- Clear the pooled
inLISboolean array immediately after renting fromArrayPool<bool>to avoid staletruevalues. - Move
List<PatchData>.Clear()from rent-time to return-time to prevent pooled lists from retaining references while sitting in the pool. - Remove a stray markdown fence in
.github/instructions/memory.instructions.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Abies/DOM/Operations.cs | Adjusts ArrayPool/List pooling behavior to reduce stale data and reference retention during diffing/batching. |
| .github/instructions/memory.instructions.md | Removes unintended trailing markdown fence content. |
| private static void ReturnPatchDataList(List<PatchData> list) | ||
| { | ||
| if (list.Count < 1000) // Prevent memory bloat | ||
| { | ||
| list.Clear(); // Clear to avoid retaining PatchData references | ||
| _patchDataListPool.Enqueue(list); | ||
| } |
There was a problem hiding this comment.
ReturnPatchDataList only clears when the list is small enough to be pooled. When list.Count >= 1000, the list isn’t enqueued but it also isn’t cleared, so PatchData references can stay alive through the rest of ApplyBatch (and possibly longer depending on JIT lifetimes), increasing peak memory usage. Consider clearing unconditionally (capture the count/capacity first to decide whether to enqueue) so references are always released promptly, regardless of pooling.
![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: 387a135 | Previous: d80f18a | Ratio |
|---|---|---|---|
Abies.Benchmarks.Diffing/AttributeOnlyDiff |
690.3276198705038 ns (± 4.0717552093608) |
623.3429936000279 ns (± 1.2350439700894407) |
1.11 |
Abies.Benchmarks.Diffing/TextOnlyDiff |
648.6550432840983 ns (± 1.7724663547281538) |
611.9344929967608 ns (± 2.2610401817071653) |
1.06 |
Abies.Benchmarks.Diffing/NodeAdditionDiff |
661.8115016497098 ns (± 1.5573554970816041) |
620.4319965998332 ns (± 2.8799777191344367) |
1.07 |
Abies.Benchmarks.Rendering/RenderWithHtmlEncoding |
761.3300703684489 ns (± 11.724409721337885) |
712.7647151265826 ns (± 1.9236195706564871) |
1.07 |
Abies.Benchmarks.Rendering/RenderWithEventHandlers |
389.2072279930115 ns (± 7.029104863604926) |
366.835517678942 ns (± 0.987870419588967) |
1.06 |
Abies.Benchmarks.Rendering/RenderLargePage |
39194.39935980903 ns (± 1624.0153750086524) |
36775.64470999582 ns (± 210.88858202674766) |
1.07 |
Abies.Benchmarks.Rendering/RenderDeeplyNested |
681.9315300721389 ns (± 7.035061063782083) |
624.6411854426066 ns (± 3.9898722445489585) |
1.09 |
Abies.Benchmarks.Rendering/RenderWideTree |
5074.469181315104 ns (± 94.82520245396003) |
4797.488340650286 ns (± 12.165921901240308) |
1.06 |
Abies.Benchmarks.Handlers/CreateSingleHandler_Message |
39.901769264177844 ns (± 1.0241859305679943) |
37.8883880575498 ns (± 0.3826140329364604) |
1.05 |
Abies.Benchmarks.Handlers/CreateSingleHandler_Factory |
57.35313802415674 ns (± 1.8562179685314877) |
50.70730579296748 ns (± 0.8727071789794933) |
1.13 |
Abies.Benchmarks.Handlers/Create50Handlers |
2755.367873265193 ns (± 72.98009773575268) |
2420.239361572266 ns (± 38.40334347791619) |
1.14 |
Abies.Benchmarks.Handlers/Create100Handlers |
4293.973270893097 ns (± 82.57304416680834) |
4038.839292086088 ns (± 25.279375393502455) |
1.06 |
Abies.Benchmarks.Handlers/CreateButtonWithHandler |
107.01654115951422 ns (± 3.418790443659466) |
98.25867835283279 ns (± 1.5228837837020186) |
1.09 |
Abies.Benchmarks.Handlers/CreateInputWithMultipleHandlers |
281.840002759974 ns (± 11.055930439620434) |
266.19114386407955 ns (± 5.795994019887518) |
1.06 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @MCGPPeters
1 participant
📝 Description
What
Address Copilot review comments from PR #59 regarding ArrayPool and List pooling memory safety.
Why
Copilot identified potential issues:
inLISbool array rented fromArrayPool<bool>.Sharedmay contain staletruevalues from previous usesList<PatchData>returned to pool without clearing retains references to PatchData objectsHow
inLISarray immediately after renting, before useList<PatchData>before returning to pool (moved from rent to return)🔗 Related Issues
Follow-up to PR #59 (LIS algorithm fix)
✅ Type of Change
🧪 Testing
Test Coverage
Testing Details
✨ Changes Made
inLIS.AsSpan(0, newLength).Clear()after renting from ArrayPoollist.Clear()fromRentPatchDataListtoReturnPatchDataList🔍 Code Review Checklist