Performance issues on forms with lists of > 150 items
Closed this issue · 25 comments
Hello!
Thanks for building this awesome library!
I love expressing the validation using a test-suite declaration style ❤️
I'm using it on a form with 3 lists of items, and when the total number of items start surpassing the 150 elements the computation time of every validation becomes > 100ms (measurement done on my laptop, a Mac M2 Pro)
This is a screenshot of the profiling of a keystroke on that form:
Are there some best practices I can follow in order to avoid incurring into these kind of performance issues?
While I guess it's normal for performance to degrade as the number of fields to validate increases, it feels that the most of the cost comes from the nesting of the various helpers (each, omitWhen, skip, only etc...) so I'm kinda wondering if there are some optimization opportunities lurking inside these utilities.
To reproduce the issue I've create this repro: https://stackblitz.com/edit/vitejs-vite-gwp4ig
Given the simplicity of the example I needed to spawn more items in order to reproduce the same performance issues.
EDIT: More complex repro here --> https://stackblitz.com/edit/vitejs-vite-mlpchh
Hey @gdorsi I just went to check the repro blitz and it seems to only contain one field. Did you maybe create in in a fork?
Hey @gdorsi I just went to check the repro blitz and it seems to only contain one field. Did you maybe create in in a fork?
Ah sorry, it was broken due to a typo on querySelector!
It is one input, but the validation runs on 500 fields.
I can try to put toghether something more complex if you need a more realistic testing ground.
Here is a more complex test, with a mix of each and omitWhen. https://stackblitz.com/edit/vitejs-vite-mlpchh
Here I've managed to get similar performance values to what I'm experiencing in my app.
Thank you for the fixed repro @gdorsi !
I started mapping out the inefficiencies. Found three main inefficiencies so far, and I have taken care of the first one. Not done yet, and it is probably going to be the smaller cost saver of this changeset - but you can already start trying it out in this version: vest@5.4.0-dev-20240719-ed80
Can you let me know if that works any better for you?
Ok, so it took some fiddling, but I think I was able to come up with something that can somewhat improve the performance of large forms.
Here is the new version number you can install: vest@5.4.0-dev-20240720-002e
Right off the bat, it should shave off some significant time. It shaved off 40% in my test.
I can probably squeeze some more perf out of it, but I will need to find where the rest is coming from.
In the meantime, let me know if that works for you. If it does, I'll wrap it nicely and publish a production build.
Here's a the profile result of a simple form with 500 inputs with the production build
And here is the same app with my new development build
Made some more progress on this issue, and now I am able to get results of anywhere between 35ms to 55ms for a 500 fields form.
This latest version can be tried using: vest@5.4.0-dev-20240720-253c
All the tests seem to pass, so I would consider this a major win.
I think there are a few more performance gains we can make, especially around internal cache invalidation on each test run. Seems like we're invalidating the cache multiple times during the lifecycle of each tests while we shouldn't really. It is minor (.3ms), but over hundreds of tests it adds up:
That's awesome @ealush!
Tested vest@5.4.0-dev-20240720-253c on my app and got around 60% improvement!
| Before | After |
Ok, one last update @gdorsi
I think I was able to get a another perf boost on this:
Should reduce response time by a few more ms. I was able to get down all the way from 127 to 35.
Lmk if you experience the same
In my case vest@5.4.0-dev-20240721-db6f has the same performance than the version before (vest@5.4.0-dev-20240720-253c)
Looks like me committing the diff yesterday closed the issue. Sorry about this.
I just released 5.4.0. Can you please confirm this works as expected on your end?
Thanks a mill!
Can confirm that the 5.4.0 lands a huge improvement on performance, our test suite pass and and I've not observed any regression so far.
Tested on the same form but with a different number of fields.
Test on form with 60 fields:
| Before | After |
Test on form with 180 fields:
| Before | After |
For now this is a great result as I don't think that there are too many forms of > 100 fields in the wild and the performance with < 100 fields now is above the 60FPS.
Still wondering why the performance degradation isn't linear on the form size.
Will dig more into it on my free time to see if I can find some other performance opportunities.
I've got a pretty good idea why this is not linear with the amount of fields.
The reason is that for each of the fields, there are some operations that traverse the entire tree, so treeSize*treeSize.
I'm looking for a way to reduce the needs of these as well, because we might not always need this kind of traversal, but at least for now - we were able to get some significant improvement.
The rest will require some rearchitecting of the test function, which I am looking into.
For now, I am closing this issue. Thanks for helping me with the fast feedback loop!
Ah good!
Thanks for the very quick fix!
I'm facing the same issue. I have a dynamic form that works well with around 100 fields (planning to go 10000+), but when I try a larger form, the performance gets worse. I created theses suites for each part of the form when I'm dealing with them individualy.
https://stackblitz.com/edit/vitejs-vite-sresjq?file=tests%2Ffields.validation.test.js
Created two test files:
fields.validation.not.nested.test.js
NN time1: 2.635ms
NN time10: 2.285ms
NN time100: 15.97ms
NN time500: 120.41ms
fields.validation.test.js (nesting the suite)
NESTED time1: 2.435ms
NESTED time10: 4.175ms
NESTED time100: 222.195ms
NESTED time500: 15.656s
Note that fields.validation.js I use a inner.fields.validation.js suite.
Hey @paulodiogo, thanks for reporting this and creating this detailed test case scenario.
I do indeed see the issue, and I believe it is related to what I replied above to @gdorsi about the increase not being linear.
One thing I think you can do, and still achieve everything you need is to nest via functions rather than suites.
Currently your suite looks like this:
const suite1 = create(() => {/*...*/})
const suite2 = create((data) => {
suite1(data)
})
If you just replace it with:
function nestedTests1(data) {
test(/*...*/);
}
const suite = create((data) => {
nestedTests1(data)
})
You should see an immediate perf boost, without losing any capability. I do want to improve the performance of nested suites (as I mentioned above), but that would require making some changes to the way the test function works, and will require more time.
Will such a solution work for you in the meantime?
Yeah! This change improved the time! Thank you!
From 23secs to 300ms @ealush
@paulodiogo, @gdorsi
I made yet another improvement to Vest's infra that should further reduce the initialization time of large suites. Can you try Vest@5.4.1 and let me know how that works?
This should do the trick: 0462bc0
@paulodiogo, @gdorsi
I made yet another improvement to Vest's infra that should further reduce the initialization time of large suites. Can you try Vest@5.4.1 and let me know how that works?This should do the trick: 0462bc0
Awesome!
I'm on vacation now and won't be able to give you a feedback before next Monday.
v.5.4.1 brings another huge performance improvement in our form 🎉
| v5.4.0 | v5.4.1 |
Test done editing a single field on a big form of 100 fields
Thanks!
Yes! Glad to hear that this worked for you. Closing the issue now as we extracted every last drop of it.
Thanks for testing reporting back each time.
Also, this is now on twitter https://x.com/evyataral/status/1823776130379272684