Do not re-insert memoized vnodes that keep their relative order after swap #4888
Conversation
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Resultscreate10kduration
usedJSHeapSize
filter-listduration
usedJSHeapSize
hydrate1kduration
usedJSHeapSize
many-updatesduration
usedJSHeapSize
replace1kduration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
text-updateduration
usedJSHeapSize
tododuration
usedJSHeapSize
update10th1kduration
usedJSHeapSize
|
|
Edit: Bit bigger now Size Change: +13 B (+0.03%) Total Size: 47.5 kB
ℹ️ View Unchanged
Thanks for the PR & thorough investigation! I'll need to take a better look tomorrow (or someone else may jump in first), but looks like you did quite a bit of research & testing on this and we appreciate it! |
There was a problem hiding this comment.
This is an awesome find, when I re-wrote the heuristics for skew based diffing I never checked the js-framework-benchmark and I even explicitly pointed out better swap performance 😅
Thank you for the thorough research, this is a really great find!
Ideally it would be a separate code path but I wanted to get your feedback first. Let me know your thoughts!
I agree with this generally, in Preact though we have a tendency to balance bytes and performance. Separating code paths often leads to relatively large byte size increases. We could prototype it and see what that balance is and I'm happy to discuss it but wanted to be clear about our philosophy.
src/diff/children.js
Outdated
| childVNode, | ||
| oldDom, | ||
| parentDom, | ||
| !(childVNode._flags & INSERT_VNODE) /* shouldSkipDomUpdate */ |
There was a problem hiding this comment.
We could probably hoist this check into a variable so we don't do it twice but don't feel strongly about that as it's just a bit check
There was a problem hiding this comment.
@JoviDeCroock Thanks for your feedback! Pushed a commit that hoists the check into a variable.
After sleeping on it, I decided to also get rid of double negation to make it simpler to read, so I renamed shouldSkipDomUpdate to shouldPlace. Please let me know if you have any preferences on the flag name or anything else.
There was a problem hiding this comment.
Re-requested approvals, will cherry-pick in another PR to 10.x right after review.
There was a problem hiding this comment.
Just as an aside: the main branch here is our upcoming v11 release, v10.x is the current release line. This probably colored your results a bit as v11 has landed some other improvements too, but by all means, this looks to be a really solid improvement (and w/ a byte reduction no less).
This change could likely also be ported to v10 w/ similar (I'm guessing/hoping) improvements there; it should be more or less a direct copy/paste, I don't think these code paths have changed in much v11.
Looks great though, and thanks again for the investigation!
Thanks for heads up! Will address comments and put up a PR to the v10 branch in a couple of hours. Also, amazing job on the V11 @rschristian @marvinhagemeister @JoviDeCroock! I did in fact see a good boost in majority of the scores relative to V10. It was big enough jump for me to realize that I need to make sure I bench against latest main otherwise it wouldn’t be a fair benchmark lol. So the scores you see are technically both V11. Exciting stuff! Would be fun to get even closer to inferno :)) |
… check into a variable
vasylenkoval
requested review from
JoviDeCroock,
rschristian and
marvinhagemeister
Description
I was looking at js-framework-benchmark and found something that looked a bit strange. All of the Preact benchmarks that use memoized rows have
swap rowsperformance similar to React, while all other benchmarks that don't use memoized rows have faster swaps but incur the cost of not being able to skip processing unchanged rows inselect row.After taking a closer look at your codebase, I was pleasantly surprised that Preact is flagging all moved nodes granularly during reconciliation as opposed to React's more optimistic handling (deopt backwards swaps). So I wanted to track down where the cost was coming from.
Debugging
I modified the
preact-hooksbenchmark to have memoized rows and made an unminified build to debug.The insert condition seems to be triggered for these 3 cases:
For most cases, if the
insertcall is made for a memoized vnode without an insert flag, it will match the old dom and skip the insert. Unfortunately, when a swap is made, it's not guaranteed to match, resulting in unnecessary calls toinsertBeforeeven though the order of vnodes between swapped items did not change.Notice how it starts re-inserting rows between the swapped ones while pushing the second (swapped) row forward, instead of skipping over nodes in between and swapping the second row at the end when it encounters it during the diff. It's happening because the second row is still in the DOM at its old position and children in between are failing to skip the dom equality check that would bail out the
insertBeforecall.preact-debug.1.mp4
Proposed change
I figured that a quick and dirty fix with the least amount of code would be to just pass a flag to
insertto disable the DOM insertion in cases when it's not required, while still keeping the logic around sCU bailout and incrementing the next oldDom pointer in tact. Ideally it would be a separate code path but I wanted to get your feedback first. Let me know your thoughts!Tests
I added a test that verifies extra
insertBeforecalls are not happening in this case. I can also add more tests around simply testing re-ordering with memoized children, however, you seemed to have a pretty decent coverage there already.Results
Here's the comparison between unmodified
preact-classesbench with and without the above commit. I chosereact-classesbecause it's using memoized rows and I did not want to compare against a benchmark that I refactored to keep it fair.Please disregard the
v10.25.2at the top, both benchmarks are using the latest build from main at this commit 68b30fe, not npm. I just swapped the preact package in node modules, the version in the bench is pulled from package.json.Other Preact benchmarks that do not use memoized rows would see even bigger shifts in overall scores if refactored.