perf: Add lazy memoization for deferred node evaluation#56
Conversation
Implements ILazyMemoNode and LazyMemo<TKey> types that defer node construction until actually needed during diffing. This provides true Elm-style lazy semantics where the factory function is only called when memo keys differ. Key changes: - Add ILazyMemoNode interface with MemoKey, CachedNode, Evaluate() - Add LazyMemo<TKey> record implementing ILazyMemoNode - Add lazy<TKey>() helper function in Elements.cs - Update DiffInternal to handle lazy memo nodes before regular memo - Add UnwrapMemoNode helper for consistent memo unwrapping - Add MemoHits/MemoMisses counters for performance analysis - Update Runtime.PreserveIds to handle lazy memo nodes - Add comprehensive unit tests for lazy memo behavior Benchmark results (Select 1k): - Before lazy: ~152.4ms median - With lazy: ~111.9ms median (27% improvement) The lazy approach skips both node construction AND subtree diffing for unchanged rows, providing significant performance gains for list-heavy UIs like the js-framework-benchmark.
| return node.Id; | ||
| } | ||
| else if (node is IMemoNode memo) | ||
| { |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general, to fix a “useless assignment to local variable” caused by a pattern variable, you either remove the unused variable or replace it with a discard (_) or a simpler type pattern, keeping only what is needed for control flow. The goal is to preserve the type test (if it’s needed) but avoid binding a name that is never read.
In this specific case, inside GetKey(Node node), the if condition currently uses a type pattern if (node is ILazyMemoNode lazyMemo), but the body of the if does not use lazyMemo at all; it only returns node.Id. To fix this without changing any behavior, replace the pattern with a simple type-only pattern: if (node is ILazyMemoNode). This preserves the logic (“if node is an ILazyMemoNode, return its Id without evaluating it”) while avoiding the useless assignment.
Concretely:
- File:
Abies/DOM/Operations.cs - Region: the
GetKeymethod around lines 1860–1870. - Change the
ifstatement fromif (node is ILazyMemoNode lazyMemo)toif (node is ILazyMemoNode). - No new imports, methods, or definitions are required.
| @@ -1862,7 +1862,7 @@ | ||
| // Handle Memo nodes by getting the key of their cached content | ||
| // For lazy memos, we need to evaluate to get the key (or use the lazy node's own Id) | ||
| Node effective; | ||
| if (node is ILazyMemoNode lazyMemo) | ||
| if (node is ILazyMemoNode) | ||
| { | ||
| // Use the lazy node's ID as the key - don't evaluate just for key lookup | ||
| return node.Id; |
|
|
||
| // Unwrap memo nodes to get the actual element ID for matching | ||
| Node effectiveNewChild; | ||
| if (newChild is ILazyMemoNode newLazyChild) |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general, to fix a "useless assignment to local variable" for a pattern variable, keep the type test but remove the unused binding. In C#, this means changing x is T v to either x is T or x is T _ when v is never referenced.
In this specific code, on line 270 we have:
if (newChild is ILazyMemoNode newLazyChild)
{
// For lazy memo, use its own ID rather than evaluating
effectiveNewChild = newChild;
}newLazyChild is not used anywhere. The existing logic only needs to know whether newChild is an ILazyMemoNode, not to access the memo node instance via the pattern variable. To preserve behavior while removing the useless assignment, update the if to use a discard pattern (or just a simple is check). The minimal, idiomatic change is:
if (newChild is ILazyMemoNode)
{
// For lazy memo, use its own ID rather than evaluating
effectiveNewChild = newChild;
}No new methods, imports, or additional definitions are required, and no other lines need to change.
| @@ -267,7 +267,7 @@ | ||
|
|
||
| // Unwrap memo nodes to get the actual element ID for matching | ||
| Node effectiveNewChild; | ||
| if (newChild is ILazyMemoNode newLazyChild) | ||
| if (newChild is ILazyMemoNode) | ||
| { | ||
| // For lazy memo, use its own ID rather than evaluating | ||
| effectiveNewChild = newChild; |
There was a problem hiding this comment.
Pull request overview
Adds lazy memoization primitives to Abies’ virtual DOM so expensive subtrees (notably list rows) can skip both node construction and diffing when memo keys are unchanged, targeting improved performance in scenarios like js-framework-benchmark “select row”.
Changes:
- Introduces
IMemoNode/ILazyMemoNodeplusMemo<TKey>/LazyMemo<TKey>node wrappers. - Extends rendering and diffing to understand memo and lazy-memo nodes, including helper unwrapping logic.
- Adds unit tests covering memo/lazy behavior and a select-row-style scenario.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| Abies/Runtime.cs | Updates PreserveIds to account for memo/lazy-memo nodes during ID preservation. |
| Abies/Html/Elements.cs | Adds public memo<TKey>(...) and lazy<TKey>(...) helpers to create memoized nodes. |
| Abies/DOM/Operations.cs | Adds memo/lazy-memo node types + integrates them into rendering and diffing logic. |
| Abies.Tests/DomBehaviorTests.cs | Adds unit tests validating memo/lazy rendering and diff behavior (hits/misses/evaluation). |
Comments suppressed due to low confidence (1)
Abies/DOM/Operations.cs:1686
- This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
foreach (var idx in keysToAdd)
{
// Unwrap memo nodes to get the actual content for patch creation
var effectiveNode = UnwrapMemoNode(newChildren[idx]);
if (effectiveNode is Element newChild)
{
patches.Add(new AddChild(newParent, newChild));
}
else if (effectiveNode is RawHtml newRaw)
{
patches.Add(new AddRaw(newParent, newRaw));
}
else if (effectiveNode is Text newText)
{
patches.Add(new AddText(newParent, newText));
}
}
| if (child is ILazyMemoNode lazyMemo) | ||
| { | ||
| oldChildrenById[childElem.Id] = child; | ||
| effectiveChild = lazyMemo.CachedNode ?? lazyMemo.Evaluate(); | ||
| } |
There was a problem hiding this comment.
This code evaluates lazy memo children (lazyMemo.CachedNode ?? lazyMemo.Evaluate()) while building oldChildrenById. In the runtime flow, dom is set from View() and Render.Html doesn’t populate CachedNode, so CachedNode will typically be null and this ends up evaluating all lazy factories on every update, defeating the purpose of lazy(). PreserveIds should avoid calling Evaluate() here; match lazy nodes using their wrapper Id (or another non-evaluating key) and only recurse into cached content when it’s already present.
| if (newChild is ILazyMemoNode newLazyChild) | ||
| { | ||
| // For lazy memo, use its own ID rather than evaluating | ||
| effectiveNewChild = newChild; | ||
| } |
There was a problem hiding this comment.
For newChild that is ILazyMemoNode, effectiveNewChild is set to the wrapper node (to avoid evaluation), but oldChildrenById is keyed using the effective child element/text id (often from evaluated content). As a result, matching by newChildElem.Id / newTextNode.Id won’t work for lazy wrappers, and ID preservation will frequently fall back to null matches. Consider keying oldChildrenById with the lazy wrapper Id (without evaluation) and matching with newLazyChild.Id consistently.
Abies/DOM/Operations.cs
Outdated
| /// <summary>Gets the memo key as object for interface implementation.</summary> | ||
| object IMemoNode.MemoKey => Key; | ||
|
|
There was a problem hiding this comment.
object IMemoNode.MemoKey => Key; will box and allocate on every access when TKey is a value type (e.g., tuple keys). Since key comparisons happen per memo node during diffing, this can add substantial allocations in a hot path. Consider caching a boxed key once (field/property initialized at construction) or exposing a strongly-typed key-comparison method to avoid per-access boxing.
| /// <summary>Gets the memo key as object for interface implementation.</summary> | |
| object IMemoNode.MemoKey => Key; | |
| // Cache the boxed key once per instance to avoid repeated boxing allocations when TKey is a value type. | |
| // This is a hot path during diffing, so we intentionally use a private field here for performance. | |
| private readonly object boxedKey = Key!; | |
| /// <summary>Gets the memo key as object for interface implementation.</summary> | |
| object IMemoNode.MemoKey => boxedKey; |
| /// <summary>Gets the memo key as object for interface implementation.</summary> | ||
| object ILazyMemoNode.MemoKey => Key; |
There was a problem hiding this comment.
object ILazyMemoNode.MemoKey => Key; has the same per-access boxing/allocation issue for value-type keys. This can significantly reduce the performance benefit of lazy memoization for common struct/tuple keys. Consider caching a boxed key once, or providing a key-equality API that can use EqualityComparer<TKey>.Default without boxing.
| /// <summary>Gets the memo key as object for interface implementation.</summary> | |
| object ILazyMemoNode.MemoKey => Key; | |
| // Cache a single boxed instance of the key to avoid per-access boxing for value-type keys. | |
| // This is a hot path in the virtual DOM diffing, so we trade a small amount of state here | |
| // for significantly reduced allocation pressure when memo keys are structs/tuples. | |
| private readonly object boxedKey = Key!; | |
| /// <summary>Gets the memo key as object for interface implementation.</summary> | |
| object ILazyMemoNode.MemoKey => boxedKey; |
| if (oldLazy.MemoKey.Equals(newLazy.MemoKey)) | ||
| { | ||
| // Keys match - skip evaluation AND diffing entirely | ||
| Interlocked.Increment(ref MemoHits); | ||
| return; |
There was a problem hiding this comment.
The unconditional Interlocked.Increment for memo hit/miss counting adds atomic ops to the memo fast-path. For large lists with many memo nodes, this overhead will show up directly in diff time. Consider guarding the counters behind a DEBUG/diagnostics flag or moving them to benchmark-only instrumentation so production diffs don’t pay this cost.
| else if (effectiveNewChild is ILazyMemoNode lazyChild && oldChildrenById.TryGetValue(lazyChild.MemoKey?.ToString() ?? newChild.Id, out var oldLazyMatch)) | ||
| { | ||
| // Try to match lazy nodes by their ID |
There was a problem hiding this comment.
This branch attempts to match lazy nodes using lazyChild.MemoKey?.ToString() ?? newChild.Id, but oldChildrenById is never populated with MemoKey.ToString() keys (it uses element/text IDs). This means the lookup will almost never succeed and can hide matching bugs. If you want to match lazy wrappers without evaluation, use the wrapper Id on both sides (or store a dedicated non-evaluating match key).
| else if (effectiveNewChild is ILazyMemoNode lazyChild && oldChildrenById.TryGetValue(lazyChild.MemoKey?.ToString() ?? newChild.Id, out var oldLazyMatch)) | |
| { | |
| // Try to match lazy nodes by their ID | |
| else if (effectiveNewChild is ILazyMemoNode lazyChild && oldChildrenById.TryGetValue(lazyChild.Id, out var oldLazyMatch)) | |
| { | |
| // Match lazy memo wrapper nodes by their own stable Id without forcing evaluation |
| // Handle Memo nodes by getting the key of their cached content | ||
| // For lazy memos, we need to evaluate to get the key (or use the lazy node's own Id) | ||
| Node effective; | ||
| if (node is ILazyMemoNode lazyMemo) |
|
|
||
| // Unwrap memo nodes to get the actual element ID for matching | ||
| Node effectiveNewChild; | ||
| if (newChild is ILazyMemoNode newLazyChild) |
There was a problem hiding this comment.
This assignment to newLazyChild is useless, since its value is never read.
| if (newChild is ILazyMemoNode newLazyChild) | |
| if (newChild is ILazyMemoNode) |
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: c6f3983 | Previous: 3dd327e | Ratio |
|---|---|---|---|
Abies.Benchmarks.Diffing/TextOnlyDiff |
653.3049531664167 ns (± 1.7040855933741181) |
621.513681778541 ns (± 0.8180922522574419) |
1.05 |
Abies.Benchmarks.Diffing/NodeAdditionDiff |
663.5258686883109 ns (± 7.988572314987832) |
611.9001653535025 ns (± 1.9121062391794073) |
1.08 |
Abies.Benchmarks.Rendering/RenderSimpleElement |
196.32321987946827 ns (± 1.0050026629118092) |
158.46328608989717 ns (± 1.706703075035911) |
1.24 |
Abies.Benchmarks.Rendering/RenderWithHtmlEncoding |
778.3191562652588 ns (± 4.426571631054409) |
671.9957122166951 ns (± 5.73949328417667) |
1.16 |
Abies.Benchmarks.Rendering/RenderSmallPage |
693.3897609710693 ns (± 3.248092220401891) |
562.0053981781006 ns (± 3.3810367366598104) |
1.23 |
Abies.Benchmarks.Rendering/RenderMediumPage |
5579.414865620931 ns (± 35.42538202246604) |
4250.59948018392 ns (± 54.22786152924198) |
1.31 |
Abies.Benchmarks.Rendering/RenderLargePage |
39890.0331624349 ns (± 589.3904674205153) |
35797.87312469482 ns (± 1264.8708965270082) |
1.11 |
Abies.Benchmarks.Rendering/RenderWideTree |
5115.797602335612 ns (± 51.70538842369631) |
4008.324872153146 ns (± 36.66617168614308) |
1.28 |
Abies.Benchmarks.Rendering/RenderComplexForm |
2437.8062693277993 ns (± 18.4805109179993) |
2135.162490081787 ns (± 22.054145850984792) |
1.14 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @MCGPPeters
📝 Description
What
Implements
ILazyMemoNodeandLazyMemo<TKey>types that defer node construction until actually needed during diffing. This provides true Elm-style lazy semantics where the factory function is only called when memo keys differ.Why
The existing
memo()function eagerly evaluates the node factory at construction time. This means that even for unchanged list items, we still pay the cost of constructing all virtual DOM nodes. For a 1000-row table where only 2 rows change during selection, we were creating 10,000+ objects unnecessarily.How
The
lazy()function wraps a factory function (Func<Node>) that is only called when:When keys match, both node construction AND subtree diffing are skipped entirely.
🔗 Related Issues
Improves js-framework-benchmark select row performance
✅ Type of Change
🧪 Testing
Test Coverage
Testing Details
LazyMemobehaviorSelect 1k Benchmark Results:
✨ Changes Made
Core Types
ILazyMemoNodeinterface - Defines lazy memo contract withMemoKey,CachedNode,Evaluate(),WithCachedNode()LazyMemo<TKey>record - ImplementsILazyMemoNodewith deferred factory evaluationAPI
lazy<TKey>(key, factory)helper inElements.cs- Creates lazy memo nodesDiffing Logic
DiffInternalto handleILazyMemoNodebeforeIMemoNodeUnwrapMemoNodehelper for consistent memo unwrappingMemoHits/MemoMissescounters for performance analysisRuntime
PreserveIdsto handle lazy memo nodes properlyTests
LazyMemoNode_ShouldDeferEvaluation- Verifies factory is not called at constructionLazyMemoNode_DiffWithSameKey_ShouldNotEvaluate- Verifies cache hit skips factoryLazyMemoNode_DiffWithDifferentKey_ShouldEvaluate- Verifies cache miss calls factoryLazyMemoNode_ShouldRenderContent- Verifies lazy nodes render correctlyLazyMemoNode_InParent_ShouldRenderCorrectly- Verifies parent/child renderingLazyMemoNode_SelectScenario_ShouldMinimizeEvaluations- Simulates benchmark scenario🔍 Code Review Checklist
dotnet format --verify-no-changespasses📊 Usage Example
For a select operation on 1000 rows: