-
Notifications
You must be signed in to change notification settings - Fork 840
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
Updated Voby to v0.7.0 #1012
Updated Voby to v0.7.0 #1012
Conversation
Thanks, especially for removing #800. Both vanillajs and voby appear to be a bit slower than in the last run. Hard to tell if it's just noise or maybe just today's OS updates. |
@fabiospampinato the memory metrics for creating/clearing 1k rows seem to be a bit of an outlier among the fast libs. might also be why "clear rows" is a bit slower than others. 🤔 |
@krausest thanks for merging and the updated results. The numbers seem about right to me, at the end of the day there are too many factors to measures things super accurately. Maybe I have made things a bit slower with the new way the selected row is handled, could be. @leeoniya yeah nodes are recycled, that's a pretty awesome optimization but it benchmarks poorly here (a bit more time spent when making rows, and that's something the benchmark measures a lot, and a bit more time spent when removing nodes), on the other hand the amount of time it takes to replace rows is super close to the vanilla implementation, despite all the extra stuff that voby does, I don't know I think it's worth it as an optimization for what I need, I could try turning it off and see what the impact on the benchmark would be. Maybe you could try that. |
It looks like I didn't fully fix an issue that I was seeing the other day 🙃 I need to spend some more time on this... |
@leeoniya Just ran the benchmark locally with recycling on and off, with recycling off the memory usage that you see after clearing rows is more reasonable. Other than that the rest seems about the same, but replacing rows is a bit slower. |
@fabiospampinato nice. in real apps i would probably keep recycling off if the difference is this small. i would rather keep the GC happy and not have a lot of retained nodes for it to track that may never be re-used for the lifecycle of a long-running app. in extreme cases it may even resemble a memory leak unless there is some kind of max lifetime retention policy (i think even something as short as 100ms or X amount of redraws would get the best of both worlds). |
@leeoniya recycling is opt-in so it would be off by default, it's not a safe optimization that can be applied under every scenario. I've thought a bit about how to delete these nodes at some point, but I couldn't think of a great solution and it seems a relatively minor detail for me at the end of the day, like we are seeing less than 1MB of difference for keeping 1k rows in the page (~5k nodes?). Like if you only enable this for simple virtualized lists, which is what I'm probably going to use it with, you'll be keeping like 50 nodes in memory for future usage maybe, it's arguably a non-problem memory-wise in many cases. |
I added some more optimizations, cleaned up the code a bit, and now I'm storing the "selected" state in the model.
I think performance might be slightly better, as N-1 no longer necessary observables have been deleted in the process, but the noise of measuring is probably higher than that anyway.
Fingers crossed now, unless there are some issues or decent optimizations missing, the framework is "done". I'll probably tweak it a little bit once I start using it and know where the pain points are though.