Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

⚡ Improves x-for performance #4361

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ekwoka
Copy link
Contributor

@ekwoka ekwoka commented Sep 3, 2024

This also solves #4192 #4157 related to using x-sort with x-for.

The core issue is that x-for never checks if any elements have moved, due to how it is optimized.

See in playground

I looked at just solving that directly, but it would definitely just increase work and increase code.

I identified that, if we want to just always ensure we put the elements in the right place, then we could also massively simplify the logic while maintaining the vast majority of the behavior and optimizations, while solving this initial problem.

Note: As I worked through this, I was quite impressed with how optimized Alpine actually was in the first place. It wasn't a joke when you said it was complexity with a purpose. I was quite worried I'd end up in a situation where the code logic was uglier, longer, slower, and there were times when the old version was doing way better at things I wasn't sure how to handle effectively. Had to do a lot of profiling to find the simplest fixes. Wrote a blog post about the steps, process, logic.

I think I got there. Smaller, faster, and I think many would agree clearer logic.

Synthetic Benchmarks

Some synthetic benchmarks that isolated just the x-for logic. This number includes the browser repaint/reflow, not purely logical gains. For most, the repaint/reflow is the same/similar for both.

Action Refactored Diff from Original
Add 10,000 Items 124.235 -20 (-14%)
Add Additional 1 Item 32.845 -27 (-45%)
Shuffle 10,001 Items 165.770 -68 (-29%)
Remove 1 Item 54.895 -5 (-8%)
Swapping 2 Items 61.250 14 (30%)
Sort 10,000 Items 142.555 -71 (-33%)
Reverse 10,000 Items 146.460 -306 (-68%)
Remove Every Other Item 42.130 -9 (-17%)
Remove Remaining 5,000 Items 16.530 -7 (-31%)

JS Framework Benchmarks

Name With Refactor Before Refactor
create rows 140.1 144.3
replace all rows 154.5 165.6
updating every 10th row 29.5 29.9
select row 51.5 56.4
swap rows 42.1 45.1
remove row 32.1 33.7
create many rows 1,169.8 1,236.7
append rows to large table 146.4 157.7
clear rows 69.1 62.9
weighted mean 2.45 2.54

Extra Huge Lists

For fun.

Action Original Refactor Difference (%)
Adding 10,000 + 10,000 Items 258 157 -39.15%
Add 100,000 Items 2,784 1,491 -46.44%
Rerender Same Items 2,863 125 -95.63%
Shuffling 100,000 Items 8,163 1,839 -77.47%
Removing Every Other Item 1,855 338 -81.78%
Removing 100,000 Items 827 115 -86.09%

@browner12
Copy link
Contributor

while I don't necessarily disagree with some of the formatting changes in here, wouldn't it be best to remove them for now to make the PR easier to review?

@ekwoka
Copy link
Contributor Author

ekwoka commented Sep 3, 2024

Sure. I think there's like 2 or 3 that just got caught up in the mix. They're quite separate from the main code so it shouldn't be too much noise though. When I get the test in, I can correct some of those.

@craigballinger
Copy link

Thanks for all your work on this! Any estimate when it might be merged in?

@simonkincaidkintronix
Copy link

I think this will fix a problem that has stumped me for a good few hours now. Just bumping to see if we can merge it in?

@Senitram666
Copy link

Senitram666 commented Jan 7, 2025

I think this will fix a problem that has stumped me for a good few hours now. Just bumping to see if we can merge it in?

you can use this to test and check about your specific issue, while we wait for the merge

#4383 (reply in thread)

@brianfeister
Copy link

brianfeister commented Jan 14, 2025

Also, btw really great work @ekwoka in terms of being so thorough with the benchmarking. 🏆

Agreed that for loops as an optimization aren't generally relevant in modern JS runtimes

Copy link

@brianfeister brianfeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brianfeister
Copy link

brianfeister commented Jan 14, 2025

I'm getting hit pretty hard by this bug.

@calebporzio can you or someone on the core team review / merge this? The sortable problem described here is a really tragic oversight in terms of merging that plugin without x-for support. Also discussed here with super basic x-sort implementation bug: #4192

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants