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

Avoid notify listener calls on field register when there's no subscriber #319

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ryank109
Copy link

@ryank109 ryank109 commented Jan 31, 2020

Hello, I'm trying to address an issue with performance in react-final-form: final-form/react-final-form#667

The only way I could do this was to separate the registration and subscription of the field, slightly modifying the current registerField api to optionally accept the subscriber.

The major performance issue that I've found with react-final-form was that it was using registerField to register for the first time to get the initial values, unregister, then register again for subscribing to the subsequent updates to the form. And each time it registered, it fired off notify subscription on all previously registered fields. So, if you are trying to render 100 fields on the screen, that's 2 registration per field and 10100 or n(n + 1) notifications, I think. :)

Anyway, I was trying to optimize this by not having to call notification on registration and avoid the second registration, so I've updated the api to what you see here. I'll PR my changes for react-final-form shortly. React-final-form changes: final-form/react-final-form#727

@ryank109 ryank109 requested a review from erikras February 3, 2020 14:37
Copy link
Member

@erikras erikras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talk to me about the performance problem. How much of an effect does this change have on the performance? How many fields do you have?

@ryank109
Copy link
Author

ryank109 commented Jun 9, 2020

@erikras The problem is prominent when you use react-final-form, because the current implementation (before silent flag) calls the register twice. And by registering, it also notifies to all other subscribers. The act of registering and notifying to each subscription is causing this performance problem.

For example, we had more than 600 fields subscribed in one screen, when the field initializes, it registers, notifies all subscribers, then removes itself for the initial state (I think act of removing also notifies the subscribers again, if I recall?), then re-registers to setup the useState hook and subscribes again. With more than 600 fields, the notify to subscriptions were 1+2+3+...600 for first state setup, (and I think this was reduced by silent flag), but the next useState hook setup is 600 fields x 600 notify calls. That was more than enough to lock the display for few seconds before anything could be freed up. I don't remember the exact numbers (it's been a while and I've actually moved on to another project), but before the optimization, it was around a minute to load the initial screen, and the optimization cut it down to about 20 seconds... and majority of 20 second was material-ui, believe it or not... the material-ui is heavy and slow, and wasn't good for rendering lots of inputs in one screen. You could say the UX on the screen is bad too, but that's what I had to work with for the time being. :)

@jedwards1211
Copy link
Contributor

jedwards1211 commented Sep 22, 2020

I'm trying to make a useFieldState hook that just subscribes to the state of the field without registering validators, notifying anything etc. But looking at the registerField source code it will trigger notifications even if I don't pass fieldConfig.

(A lot of my code I'm migrating from redux-form relies on using a custom hook to get a field value and then conditionally render stuff based upon that)

Right now it seems like the only way to make this hook is to use a custom mutator to poke the subscription and listener into fieldSubscribers, which isn't super ideal but I'm gonna do it until I have a better option.

@jedwards1211
Copy link
Contributor

jedwards1211 commented Sep 22, 2020

@erikras I tried my approach outlined above but there's no way to subscribe to the value of an individual field if no <Field> is mounted for it (or useField registered it). Even if a value is set for that name in the form's values.

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

Successfully merging this pull request may close these issues.

None yet

3 participants