-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Performance regression in react-hook-form from 4.4 to 4.5 #46948
Comments
More context and a larger repro from @kmoschcau:
|
Manually bisecting between 4.4.4 and 4.5.2:
Uh, ok, so:
The good news is this seems to be fixed in the nightly, but if the fix looks safe, we should try to port it to a 4.5 patch, because the perf difference here is bananas. |
The fix is #46599, which never got ported to release-4.5. @DanielRosenwasser recalls this being probably intentional for safety—that could be, but (a) I don’t recall that, and (b) I don’t think we knew that it fixed a regression introduced around 4.5-beta. |
@kmoschcau I haven’t tried your repo yet—does typescript@next perform better for you? |
@andrewbranch Oh yes it does! Back to the old performance of 4.4.4.
|
Also |
We should definitely investigate adding an older version of react-hook-form to the perf suite. |
Thanks for releasing the fix in Extended diagnostics
|
I’m not 100% sure this is what’s going on, but I believe what’s happening is that 4.4 was giving up while resolving deep constraints (mistakenly thinking the constraints were infinitely deep), whereas 4.5+ recognizes that those constraints are not infinite, just deeper than 5 levels, and so it gets farther with actually checking correctness. |
I’m able to cut the time down from 4.66s to 1.19s by adding variance annotations to just |
I’m pretty sure the slowdown is caused by #41821—we are just able to get deeper into structural comparisons for variance computations, which ultimately are fruitless for these recursive types, but in general fixed bugs. We added variance annotations for exactly this case, and it looks like they’re going to work incredibly well here. I doubt there’s anything else actionable for us (except perhaps to help react-hook-form add variance annotations—I haven’t checked whether the problem still exists in their latest/alpha versions), but am going to get a second opinion on that tomorrow with @amcasey. |
I’m going to go ahead and close this—the performance of the library is much better in react-hook-form@beta, and also adding the variance annotations to the one type fixes the regression, so I think that’s an outcome we’re ok with. |
...
...
Originally posted by @atsikov in #46735 (comment)
The text was updated successfully, but these errors were encountered: