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

Poor performance when (over)using setFieldValue/setFieldTouched #4382

Closed
1 of 5 tasks
bgoscinski opened this issue Jul 20, 2023 · 5 comments
Closed
1 of 5 tasks

Poor performance when (over)using setFieldValue/setFieldTouched #4382

bgoscinski opened this issue Jul 20, 2023 · 5 comments
Labels
✨ enhancement a "nice to have" feature 💨 performance A performance issue or gains

Comments

@bgoscinski
Copy link
Contributor

bgoscinski commented Jul 20, 2023

What happened?

I have a large form (over 500 inputs), where each input can be edited on its own, but there's also a possibility to "batch" edit multiple fields. When using this batch edit mode I'm calling setFieldValue for each field. Also, when using batch edit I'd like to "propagate" touched status to each affected field.

It all works fine, except that there's a noticeable lag. While I'm aware that I could use v-model.lazy or similar technique to minimize the number of setFieldValue calls I think it would be cool to also improve the perf in vee-validate and make it go brrrr 🚀

I created a perf profile during editing and bluring batch inputs and it shows that the offending code is findPathState and the helper functions that it calls:
image

I think it could be optimized. Right now it calls pathStates.value.find which runs normalizeFormPath for both operands on each iteration which is wasteful because the path that we're looking for could be normalized once, and the paths on already existing PathStates could be already normalized. I have couple ideas how it could be improved:

  1. Instead of keeping pathStates in an array we could use a dictionary object or even a Map<string, PathState> where keys are already normalized paths. This would reduce number of normalization calls needed to just one - only the argument of the findPathState would need to be normalized and then used to lookup the dictionary/Map
  2. If, for some reason, it's not feasible to keep PathStates in a dict/Map we could leave it in an array but make PathState.path be already normalized path (alternatively, we could store normalized one in a new field, for example PathState.normalizedPath) and then
    - const pathState = typeof path === 'string' ? pathStates.value.find(s => isPathsEqual(s.path, path)) : path;
    + if (typeof path !== 'string') {
    +   return path  // already a path state
    + }
    + const normalized = normalizeFormPath(path)
    + const pathState = pathStates.value.find(s => s.path /* or s.normalizedPath */ === normalized) 
  3. We could memoize normalizeFormPath

I'm willing to submit a PR implementing one of these if you'd like. Just let me know which direction you think is best.

Reproduction steps

  1. Open repro
  2. Edit 'ALL foo'/'ALL bar'/'ALL baz' input
  3. Notice lag

Version

Vue.js 3.x and vee-validate 4.x

What browsers are you seeing the problem on?

  • Firefox
  • Chrome
  • Safari
  • Microsoft Edge

Relevant log output

No response

Demo link

https://stackblitz.com/edit/vee-validate-v4-checkboxes-twbzx2?file=src%2FApp.vue

Code of Conduct

@logaretm logaretm added 💨 performance A performance issue or gains ✨ enhancement a "nice to have" feature labels Jul 20, 2023
@logaretm
Copy link
Owner

Thanks for reporting this. I was concerned about using arrays due to the perf issue you mentioned here.

But the problem with field paths is that they can change, a field name/path can change. So object/Map keys will need to be kept in sync with the fields' current names. Actually <= 4.9 did use that technique of keeping an object for field names and switching to that version reduces the lag considerably, might be a workaround if it is a deal-breaker for you till I figure out how to optimize path states.

As for your second suggestion, that is possible to do I believe. It will need to be done at all entry levels for field paths, but it is doable.

@logaretm
Copy link
Owner

logaretm commented Jul 20, 2023

Found a reasonable workaround, we generate a computed property that has the field paths mapped in a map/dict. This guarantees the keys are always up to date and shouldn't perform array finds as often.

Also reduces the normalization path calls, thanks for reporting this again and for the great investigation.

@logaretm
Copy link
Owner

Did another thing, instead of generating the path map/lookup every time, I debounce it by the next tick while at the same time doing an O(1) operation to ensure the map is updated, this means it only generates the lookup once for all 600 fields in your example, it basically waits for the fields to stabilize before generating it, with the earlier commit it ran it 600 times which isn't ideal and wasteful.

@bgoscinski
Copy link
Contributor Author

Well... That was quick! Thanks 😁

Out of curiosity: in what scenarios field name/path can change? I could think of only one - when it's in an array and it (or it's ancestor) is moved to lower or higher index by remove/insert of another element or move/swap etc. Are there other scenarios that can affect field name/path?

@logaretm
Copy link
Owner

You are correct, it is mostly in loops or conditional rendering (if the components don't have a key prop). Also some developers may decide to change a field name based on some arbitrary conditions so we can't really limit it to specific scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement a "nice to have" feature 💨 performance A performance issue or gains
Projects
None yet
Development

No branches or pull requests

2 participants