fix: pair unkeyed children front-to-front during reconciliation#4099
fix: pair unkeyed children front-to-front during reconciliation#4099Madoshakalaka wants to merge 6 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 23cb183): https://yew-rs-api--pr4099-fix-unkeyed-reconcil-8duxatdr.web.app (expires Fri, 10 Apr 2026 06:34:59 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}
packages/yew/src/dom_bundle/blist.rs
Outdated
| // `rights` is stored in reverse render order. Flip to render order so | ||
| // that simple index-based pairing matches children front-to-front. | ||
| rights.reverse(); |
There was a problem hiding this comment.
Does it even make sense to keep rights in reverse order if we do this?
The only reason I can recall why this is done so that no reordering or swapping of the elements in the vec needs to happen here. You need to check for yourself that the existing implementation is the only possible way to do this with Vec<_> and the constraint that the writer calls need to happen back-to-front.
I remember trying out to use a VecDeque so that the drain above could happen at the back of the DOM order, which seemed not worth it for code size (VecDeque and Vec seem to not codeshare as much as I thought). I fully agree with your reasoning though: If the common failure case (mismatching lengths) leads to overhead downstream, then paying for two list reversals instead should be worth it. I just think that in the case of matching list lengths, no such reversing should need to take place.
There was a problem hiding this comment.
trying something in 8f3873d
Couldn't get the apply_keyed algorithm to work with render-order in a pinch.
Will experiment more.
There was a problem hiding this comment.
wait. I see a small but constanst ~0.173 kB size increase across the example crates. will fix.
There was a problem hiding this comment.
maybe it's acceptible.
There was a problem hiding this comment.
I will try to see if I can find out what causes it, too, but it's very hard to inspect the resulting binaries in such detail 😓 could be a new panic path and error message, ~170 bytes is not a lot.
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.
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