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

Slow performance with large arrays #2043

Open
tedeschia opened this issue Jul 2, 2023 · 6 comments
Open

Slow performance with large arrays #2043

tedeschia opened this issue Jul 2, 2023 · 6 comments

Comments

@tedeschia
Copy link
Contributor

tedeschia commented Jul 2, 2023

I need to validate some large arrays (about 100,000 items). When there are lots of validation errors, the validation process takes a long time to run.

I reproduced the issue in CodeSandbox: https://codesandbox.io/s/yup-performance-test-6jxn5m?file=/src/index.js

There I defined an array schema of required numbers and run the validation for some datasets to measure the performance. This is the result:

Validation of 5000 items took 169.6 ms
Validation of 10000 items took 320.1 ms - 88.7% increase vs last fun
Validation of 20000 items took 985.1 ms - 207.7% increase vs last fun
Validation of 40000 items took 5,034.8 ms - 411.1% increase vs last fun

It seems to be an exponential increase of time to process the validations.

Doing some profiling, it seems that the issue is in this line of ValidationError class:
this.inner = this.inner.concat(err.inner.length ? err.inner : err);

@jquense
Copy link
Owner

jquense commented Jul 2, 2023

That would probably be pretty easy to optimize if you want to take a stab at it?

@jquense
Copy link
Owner

jquense commented Jul 2, 2023

Alternatively, you could at abortEarly: true to fail fast, since it's unlikely you need 40,000 of probably the same error

@tedeschia
Copy link
Contributor Author

tedeschia commented Jul 2, 2023

abortEarly is not an option for me. It's nos necessarily all the same errors (this case was just an example) and I need to give the user a feedback for every row.

I tried with this change and it was solved:

const innerErrors = err.inner.length ? err.inner : [err];
this.inner.splice(this.inner.length, 0, ...innerErrors);

do you want me to send a PR?

I'm also measuring other bottlenecks and found that ValidationError constructor also takes its time (considering thousands of it) at:
super()
Error.captureStackTrace(this, ValidationError)
I'm wondering if I could implement any kind of "minimalistic" version of ValidationError class, in case of people (like me) that prioritize performance over functionallity (I'm just interested in error message, and don't use stack trace at all).
...or maybe this only happens in dev mode? I'm not sure.

@jquense
Copy link
Owner

jquense commented Jul 2, 2023

I'd definitely be open to a PR for both, maybe we could toggle the stack traces off via an option, tbh the stack isn't really useful for validation error

@tedeschia
Copy link
Contributor Author

With the proposed optimizations in #2044, the performance is significantly improved, and there is no exponential performance degradation when the array grows.
But anyway, it takes some time to perform the validations. In my tests, an array of 60000 items takes about 4 seconds to get validated, and during this process, the UI gets blocked.
I'm wondering if processing the validations in chunks, freeing the thread between them, could benefit the user experience even when the validation needs some time to process. I would explore this if you consider it a good idea.

jquense added a commit that referenced this issue Jul 28, 2023
* fix: performance improvement (#2043)

* Update src/ValidationError.ts

Co-authored-by: Jason Quense <monastic.panic@gmail.com>

* Apply suggestions from code review

Co-authored-by: Jason Quense <monastic.panic@gmail.com>

* fix PR comments

---------

Co-authored-by: Jason Quense <monastic.panic@gmail.com>
jquense added a commit that referenced this issue Jul 28, 2023
jquense added a commit that referenced this issue Jul 28, 2023
jquense added a commit that referenced this issue Aug 28, 2023
* fix: performance improvement (#2043)

* Update src/ValidationError.ts

Co-authored-by: Jason Quense <monastic.panic@gmail.com>

* Apply suggestions from code review

Co-authored-by: Jason Quense <monastic.panic@gmail.com>

* fix PR comments

* #2043 add Error toStringTag to custom ValidationError class

---------

Co-authored-by: Jason Quense <monastic.panic@gmail.com>
tedeschia added a commit to tedeschia/yup that referenced this issue Nov 24, 2023
tedeschia added a commit to tedeschia/yup that referenced this issue Nov 29, 2023
tedeschia added a commit to tedeschia/yup that referenced this issue Nov 29, 2023
@tedeschia
Copy link
Contributor Author

Hi @jquense, this is me again with this performance improvement!
I just sent the PR #2142 adding a new ValidationError class to be used when disableStackTrace=true, so it keeps compatibility with previous versions, while improving error creation performance when disableStackTrace is used.
Please could you review it?
Regards!

tedeschia added a commit to tedeschia/yup that referenced this issue Mar 4, 2024
jquense pushed a commit that referenced this issue Mar 6, 2024
…n performance (#2142)

* #2043 add ValidationErrorNoStack to improve error creation performance

* Consolidate

* #2043 remove ValidationErrorNoStack references

---------

Co-authored-by: jquense <jquense@ramp.com>
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

No branches or pull requests

2 participants