fix: pair unkeyed children front-to-front during reconciliation#4099
fix: pair unkeyed children front-to-front during reconciliation#4099Madoshakalaka merged 8 commits intomasterfrom
Conversation
`apply_unkeyed` paired nodes from the back of render order, so growing or shrinking a list destroyed and recreated leading siblings instead of reusing them. Flip `rev_children` to render order before diffing, drain/add excess from the tail, and flip back. This makes positional reconciliation match the intuitive front-to-front expectation and fixes stateful elements (`<video>`, `<input>`) being reset when a sibling list changes length.
Madoshakalaka
changed the title
|
Visit the preview URL for this PR (updated for commit 9aa56fc): https://yew-rs-api--pr4099-fix-unkeyed-reconcil-8duxatdr.web.app (expires Wed, 15 Apr 2026 18:08:57 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Benchmark - coreYew MasterPull Request |
Size ComparisonDetails
✅ None of the examples has changed their size significantly. |
Benchmark - SSRYew MasterDetails
Pull RequestDetails
|
|
The benchmark deltas are CI environmental noise, not caused by this change.
For benchmarks that do hit The real performance cost of this fix is those three in-memory reverses per |
|
@WorldSEnder you might be interested |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Replace rev_children (reverse render order) with children (render order). apply_keyed assigns reverse indices to the spliced middle so the existing run algorithm works unchanged. shift iterates in reverse. Hydration drops a now-unnecessary reverse.
Madoshakalaka
force-pushed
the
fix/unkeyed-reconciliation-front-pairing
branch
from
aada136 to
8f3873d
Compare
Replace the reversed-index hack (middle_count - 1 - fwd_idx) with native render-order indices in apply_keyed's run algorithm. The barrier tracking uses Option<usize> so checked_sub handles the "everything committed" edge case that previously required reverse indexing.
Use into_iter().rev() (pointer arithmetic on DoubleEndedIterator) instead of in-place slice reverse (element-sized swap loops).
…n path" This reverts commit 6b23703.
Resolve conflict in blist.rs where Edition 2024 migration (#4114) changed `if let` to `match` while our branch restructured the inner keyed-reconciliation logic for render-order indices. Take master's `match ancestor` syntax with our Option<usize> barrier_idx and flipped comparison directions.
- remove a duplicate call to `matching_len`. It makes no difference if we do this before or after already processing the tail if no keys are duplicate - could use `v.key()` instead of `key!(v)` when matching lengths for a handful of bytes.
WorldSEnder
left a comment
WorldSEnder
left a comment
There was a problem hiding this comment.
Down to less than 100 bytes delta for most examples, it's the best I can do. I think the additional bytes come from a call to reverse, but I don't see a way to avoid it and the perf wins trump the code size delta.
| "prepending to a keyed list should preserve trailing <e>", | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
One last comment before signing this off. I don't see the unkeyed tests as a public guarantee. They still serve as a good documentation of the current behaviour and marker for this PR and the old one stayed for years, so leave them in. If you ever find this PR and comment in the future remember that keys are the only public guarantee about node identity we make. This tests a performance modification that we think a lot of people are hitting in practice where a specific unkeyed strategy aligns better with expectations. It is NOT a semantic guarantee. Changes are okay, but should adjust the test cases.
|
Thanks, looking good! |
2 participants
Description
BList::apply_unkeyedstored children in reverse render order (rev_children) and paired old/new nodes from the back. When the child count changed between renders the pairing was wrong: growing a list by one at the tail caused the first child to be destroyed and recreated, while shrinking removed the first child instead of the last.This destroyed stateful DOM elements (
<video>,<input>) even though they never changed, and is the root cause behind the{for expr},{vec}, and{option}expression flattening regressions reported in #3262.The fix flips
rev_childrento render order, drains/adds excess from the tail, patches paired nodes front-to-front, then flips back. The only added cost is three in-memoryVec::reversecalls (pointer swaps, sub-microsecond for typical child counts) per reconciliation; DOM operation count is unchanged.What this fixes
{for iter},{vec}, and{option}expressions that flatten into a parentVListno longer corrupt leading siblings.What this does not fix
{for expr}/{vec}/{option}still flatten into the parent (macro-level issue, separate fix).for x in iter { }syntax for full correctness).Fixes #3262
Checklist
wasm_bindgen_testnode-identity tests (node_identity_testsinblist.rs) that assert DOM node reuse viaNode::is_same_node. Each test fails before the fix and passes after:for_iterable_preserves_sibling_identityvec_expression_preserves_sibling_identityoption_expression_preserves_sibling_identityunkeyed_grow_preserves_leading_nodesunkeyed_shrink_preserves_leading_nodes