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 with huge form #529

Closed
luisrudge opened this Issue Jan 11, 2016 · 126 comments

Comments

@luisrudge
Contributor

luisrudge commented Jan 11, 2016

Hi! I'm having some perf issues with a huge form I'm dealing with.
The form has 83 fields (yeah.. I know.. but it is what it is) and typing on any input is impossible, as the input lags/hangs to the point where it becomes unusable.
For each key pressed, my component is rerendering. Is that the correct behavior?

I tested with the redux-form demo website and it lags as well if you add enough fields to it:

mwwgfj1

See how the text appear in blocks of text, instead of one key after another. I captured this with 24fps, so it's not a gif issue.

@kristian-puccio

This comment has been minimized.

kristian-puccio commented Jan 11, 2016

Have you tried this in production mode with the dev tools turned off?

On 12 January 2016 at 06:08, Luís Rudge notifications@github.com wrote:

Hi! I'm having some perf issues with a huge form I'm dealing with.
The form has 83 fields (yeah.. I know.. but it is what it is) and typing
on any input is impossible, as the input lags/hangs to the point where it
becomes unusable.
For each key pressed, my component is rerendering. Is that the correct
behavior?

I tested with the redux-form demo website and it lags as well if you add
enough fields to it:

[image: mwwgfj1]
https://cloud.githubusercontent.com/assets/941075/12243471/dc57edb8-b885-11e5-81fe-4c8f5fdaed39.gif

See how the text appear in blocks of text, instead of one key after
another. I captured this with 24fps, so it's not a gif issue.


Reply to this email directly or view it on GitHub
#529.

@kristian-puccio

This comment has been minimized.

kristian-puccio commented Jan 11, 2016

Also have you added pureRenderMixin to all the inputs?

I seperate each input into it's own component and make sure I use
pureRenderMixin so only the component that is changing gets updated

On 12 January 2016 at 07:24, Kristian Puccio kristian.puccio@gmail.com
wrote:

Have you tried this in production mode with the dev tools turned off?

On 12 January 2016 at 06:08, Luís Rudge notifications@github.com wrote:

Hi! I'm having some perf issues with a huge form I'm dealing with.
The form has 83 fields (yeah.. I know.. but it is what it is) and typing
on any input is impossible, as the input lags/hangs to the point where it
becomes unusable.
For each key pressed, my component is rerendering. Is that the correct
behavior?

I tested with the redux-form demo website and it lags as well if you add
enough fields to it:

[image: mwwgfj1]
https://cloud.githubusercontent.com/assets/941075/12243471/dc57edb8-b885-11e5-81fe-4c8f5fdaed39.gif

See how the text appear in blocks of text, instead of one key after
another. I captured this with 24fps, so it's not a gif issue.


Reply to this email directly or view it on GitHub
#529.

@luisrudge

This comment has been minimized.

Contributor

luisrudge commented Jan 11, 2016

It lags in prod mode too (without devtools or redux-logger).

@luisrudge

This comment has been minimized.

Contributor

luisrudge commented Jan 11, 2016

I'm not using pureRenderMixin though. I'll try that and let you know.

@erikras

This comment has been minimized.

Owner

erikras commented Jan 11, 2016

I did my best to only change the field objects that change with each action, but if I've messed up, all the fields might be trying to rerender or something like that. Some printouts should tell you. If that's the case, it's a redux-form bug. #375 suggests that it is.

@luisrudge

This comment has been minimized.

Contributor

luisrudge commented Jan 12, 2016

Yeah.. Even implementing a custom shouldComponentUpdate, it still lags really hard. :(

shouldComponentUpdate(nextProps) {
  const currentProps = _.pick(this.props, 'defaultValue', 'initialValue', 'valid', 'active', 'touched', 'value');
  const otherProps = _.pick(nextProps, 'defaultValue', 'initialValue', 'valid', 'active', 'touched', 'value');
  return !_.isEqual(currentProps, otherProps);
}

So, the thing is.. All my inputs are trying to re-render (it doesn't because of shouldComponentupdate) even when you only focus/blur on any input.. Is #375 related to that?

@luisrudge

This comment has been minimized.

Contributor

luisrudge commented Jan 14, 2016

@erikras @kristian-puccio do you guys have any idea how I can mitigate that for the time being?

@kristian-puccio

This comment has been minimized.

kristian-puccio commented Jan 14, 2016

you could always try to debounce the onChange event through the lodash
debounce. But then you'll have to track the state locally so you can see
the updates as you type.

There was someone else doing the same thing in the issues
#454

On 14 January 2016 at 13:08, Luís Rudge notifications@github.com wrote:

@erikras https://github.com/erikras @kristian-puccio
https://github.com/kristian-puccio do you guys have any idea how I can
mitigate that for the time being?


Reply to this email directly or view it on GitHub
#529 (comment).

@erikras

This comment has been minimized.

Owner

erikras commented Jan 14, 2016

This issue, and others, like #544, is making me reconsider whether or not to use ImmutableJS internally. The complexity of maintaining nested object instances is quite difficult, and all that would go away with ImmutableJS.

@luisrudge

This comment has been minimized.

Contributor

luisrudge commented Jan 14, 2016

@erikras that sounds like lots of effort though.. are you sure it will solve this issue?

@damassi

This comment has been minimized.

damassi commented Jan 14, 2016

Immutable persistent data structures FTW. I think it would simplify a lot of tasks going forward once the initial scaffolding is in place

@kristian-puccio

This comment has been minimized.

kristian-puccio commented Jan 15, 2016

Be interesting how you choose to integrate it if you do.

Peer Dependancy or Dependancy.

Do you iterate with the api via the immutable api? I use immutable already
so for me it would be a great change but for someone who doesn't?

But then I sort of feel like redux-form is quite a sophisticated project
not the sort of thing your going to pickup as a beginner as you have to
know react + then also redux before you even start.
So if the project is aimed at developers with that level of experience
already then immutable.js isn't a hard thing to grasp and quite likely to
be in use already.

On 15 January 2016 at 10:15, Christopher Pappas notifications@github.com
wrote:

Immutable persistent data structures FTW. I think it would simplify a lot
of tasks going forward once the initial scaffolding is in place


Reply to this email directly or view it on GitHub
#529 (comment).

@erikras

This comment has been minimized.

Owner

erikras commented Jan 17, 2016

In addition to some sanity-checking tests (see commits above), I have looked into the ImmutableJS idea.

I don't see that much benefit to it, since I will still have to do all the tree-traversal (via field strings) I'm already doing, and I'm already sharing as much memory (not deep copying) as possible, via the ... spread operator.

And adding a dependency would be a big deal, not to mention that it would require users of the library to take the ImmutableJS state into account when serializing and deserializing the store.

I think I've talked myself out of it for now. Arguments in either direction are welcome.

erikras added a commit that referenced this issue Jan 17, 2016

@erikras

This comment has been minimized.

Owner

erikras commented Jan 17, 2016

@luisrudge Does the debounce suggestion help?

Another thing you could try is removing the onChange and only updating the redux store onBlur. Like this: #293 (comment)

@wmertens

This comment has been minimized.

Contributor

wmertens commented Jan 17, 2016

I think some profiling would be good, perhaps something in the component is
causing the issue…

On Sun, Jan 17, 2016, 10:00 AM Erik Rasmussen notifications@github.com
wrote:

@luisrudge https://github.com/luisrudge Does the debounce suggestion
help?

Another thing you could try is removing the onChange and only updating
the redux store onBlur. Like this: #293 (comment)
#293 (comment)


Reply to this email directly or view it on GitHub
#529 (comment).

Wout.
(typed on mobile, excuse terseness)

@steida

This comment has been minimized.

steida commented Jan 17, 2016

onBlur, caching, immutable.js, I suppose nothing can help for huge forms, or will be half broken, and definitely will be pita for debugging. Imagine long list of editable items, we have two options.

  1. connect every item form with data, basically use observer pattern
    Every hoc must use observer pattern. And what if we want to read values else where? Use form hoc everywhere? Observers are performance killers.

  2. pass everything through props and leverage pure components

I am using immutable js and pure components in https://github.com/este/este a lot, and I always ended with custom forms implementation, which sucks ofc, but nothing else helped for everything editable apps. Maybe it's time to revisit redux-form pattern.

@wmertens

This comment has been minimized.

Contributor

wmertens commented Jan 17, 2016

Like I said, profile. The whole React app uses the same principle and is
not slow so likely there are good opportunities for optimization.

On Sun, Jan 17, 2016, 5:03 PM Daniel Steigerwald notifications@github.com
wrote:

onBlur, caching, immutable.js, I suppose nothing can help for huge forms,
or will be half broken, and definitely will be pita for debugging. Imagine
long list of editable items, we have two options.

  1. connect every item form with data, basically use observer pattern
    Every hoc must use observer pattern. And what if we want to read values
    else where? Use form hoc as well?

  2. pass everything through props and leverage pure components

I am using immutable js and pure components in
https://github.com/este/este a lot, and I always ended with custom forms
implementation, which sucks ofc, but nothing else helped for everything
editable apps. Maybe it's time to revisit redux-form pattern.


Reply to this email directly or view it on GitHub
#529 (comment).

Wout.
(typed on mobile, excuse terseness)

@erikras

This comment has been minimized.

Owner

erikras commented Jan 17, 2016

@steida The "what if every input was connected to only its slice of the redux store?" idea crossed my mind. I presume that's what you mean by your first option. Seems messy as hell.

Everything through props is sort of how it's done already...

@wmertens By profiling, do you mean in Chrome devtools, or is there a better tool?

@wmertens

This comment has been minimized.

Contributor

wmertens commented Jan 17, 2016

Devtools and react perf

On Sun, Jan 17, 2016, 6:05 PM Erik Rasmussen notifications@github.com
wrote:

@steida https://github.com/steida The "what if every input was
connected to only its slice of the redux store?" idea crossed my mind. I
presume that's what you mean by your first option. Seems messy as hell.

Everything through props is sort of how it's done already...

@wmertens https://github.com/wmertens By profiling, do you mean in
Chrome devtools, or is there a better tool?


Reply to this email directly or view it on GitHub
#529 (comment).

Wout.
(typed on mobile, excuse terseness)

@erikras

This comment has been minimized.

Owner

erikras commented Jan 17, 2016

I'm having trouble getting my head around the Chrome perf tools. Anyone willing to post a graph or readout of what happens when the pound on the keyboard like the screenshot above would be appreciated.

@wmertens Wout, stop replying by email! 😡 😛

@luisrudge

This comment has been minimized.

Contributor

luisrudge commented Jan 17, 2016

I can do that!

@erikras

This comment has been minimized.

Owner

erikras commented Jan 17, 2016

I looked up @luisrudge on Wikipedia, but it forwarded me to this page.

😛 ❤️ 😄

@luisrudge

This comment has been minimized.

Contributor

luisrudge commented Jan 17, 2016

damn.. 💭

@erikras

This comment has been minimized.

Owner

erikras commented Jan 17, 2016

🎸 Rock on, LR!

@steida

This comment has been minimized.

steida commented Jan 17, 2016

@erikras I like your avatar. Same here .)

@luisrudge

This comment has been minimized.

Contributor

luisrudge commented Jan 17, 2016

image

First event is focus, second one is keypress.

@erikras

This comment has been minimized.

Owner

erikras commented Jan 17, 2016

Okay, and what does that tell us?

It might be too much to ask, but I'd love a declaration of "It spends 80% of the time in X subroutine" conclusion.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Apr 21, 2016

It turns out that React is not so happy about <Something.another.other/>. I think they have prohibited the dot syntax in JSX like that.

Are you sure? Works for me:

http://babeljs.io/repl/#?evaluate=true&presets=es2015%2Creact&experimental=false&loose=false&spec=false&code=%3CFoo.bar.wat%20%2F%3E

<Foo.bar.wat />
"use strict";

React.createElement(Foo.bar.wat, null);
@davidkpiano

This comment has been minimized.

davidkpiano commented Apr 21, 2016

@gaearon I think he might be talking about getters rather than static properties? (If I remember correctly from the V5/6 proposal thread)

@erikras

This comment has been minimized.

Owner

erikras commented Apr 21, 2016

Okay, so my problem was not the dots; it was the lowercase. I was trying to do <firstName.textarea/>, and JSX wants to the component to be title case.

I suppose something like <Fields.contact.shipping.street.input/>

And I do wonder if those dots could be done with a lazy getter... It might require some more research.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Apr 21, 2016

Okay, so my problem was not the dots; it was the lowercase. I was trying to do <firstName.textarea/>, and JSX wants to the component to be title case.

I think this is only true without dots. e.g. <firstName /> will be assumed to be a custom (web) component. But <firstName.input /> will work.

http://babeljs.io/repl/#?evaluate=true&presets=es2015%2Creact&experimental=false&loose=false&spec=false&code=%3CfirstName%20%2F%3E%3B%0A%3CfirstName.input%20%2F%3E%3B%0A%3Cprops.firstName.input%20%2F%3E%3B

<firstName />;
<firstName.input />;
<props.firstName.input />;
"use strict";

React.createElement("firstName", null);
React.createElement(firstName.input, null);
React.createElement(props.firstName.input, null);
@gaearon

This comment has been minimized.

Contributor

gaearon commented Apr 21, 2016

This also doesn’t print any warnings for me: https://jsfiddle.net/fvbaqb0t/

@gaearon

This comment has been minimized.

Contributor

gaearon commented Apr 21, 2016

Whether those are getters or static properties shouldn't matter because it happens at compilation time when we don't know that anyway.

@erikras

This comment has been minimized.

Owner

erikras commented Apr 21, 2016

Yep. Works with getters.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Apr 21, 2016

Hopefully you don't have to rewrite v6 once again now :P

@wmertens

This comment has been minimized.

Contributor

wmertens commented Apr 21, 2016

I think this can totally work both ways, if you provide fields they will be set as getters for custom components, and if you use the name attribute you add fields ad-hoc?

@erikras

This comment has been minimized.

Owner

erikras commented Apr 21, 2016

@wmertens That was the original plan, with the field getters just being wrappers for the Field components. I'm having trouble convincing myself that it's worth the extra effort to provide the wrappers. Convince me. What is really gained?

@gaearon

This comment has been minimized.

Contributor

gaearon commented Apr 21, 2016

I’d say string-based APIs are a thing to avoid, in general, because of possible renames and implicit dependencies.

@erikras

This comment has been minimized.

Owner

erikras commented Apr 21, 2016

@gaearon Right, but the string field names have to be fed into the library at some point. Why better at the outer form level (root) than down at the field level (leaves of the tree)?

@davidkpiano

This comment has been minimized.

davidkpiano commented Apr 21, 2016

@gaearon On the other hand, using string-based paths in an API are the simplest alternative to having to provide a getter and setter:

// compare this...
<Field model="foo.bar">

// to this, which is the minimum API required for no strings
const barModel = {
  get: (state) => state.foo.bar,
  set: (state, value) => ({ ...state, foo: { ...state.foo, bar: value } })
};

<Field model={barModel}>
@gaearon

This comment has been minimized.

Contributor

gaearon commented Apr 21, 2016

You know better! 👍
I thought this was something desirable, but if not, then it’s not worth the hassle 😄 . Can add it later anyway.

@wmertens

This comment has been minimized.

Contributor

wmertens commented Apr 22, 2016

Convince me. What is really gained?

@erikras the simplest way to implement it would be to walk the fields array and create properties on Field that are just (props) => <Field {...props} name={name}/>.

Advantages:

  • You cannot misspell field names, that would result in undefined element errors
  • Less typing, somewhat easier to read since the field names are in predictable locations

Disadvantages:

  • Extra code to maintain
  • Wrapper elements unless you make a FieldFactory

…hmm, I'm not really making a compelling case here :)

@erikras

This comment has been minimized.

Owner

erikras commented Apr 22, 2016

@wmertens I definitely think it's better looking code. I mean...

<div>
  <firstName.text/>
  <firstName.error className="my-customer-error-class"/>
</div>

...is very pretty.

So I think it will get implemented eventually, but maybe as a 6.x feature release in the somewhat distant future.

@ooflorent ooflorent added this to the next-6.0.0 milestone Apr 29, 2016

@davidhtien

This comment has been minimized.

davidhtien commented May 1, 2016

I get really serious lag when typing in a textarea in my form, is there a way around this? I tried what as suggested here https://github.com/erikras/redux-form/issues/293#issuecomment-157738158 but then nothing appears in the textarea when I type.

<div>
  <div className="toggle-title">Comments</div>
  <textarea className="comments-textarea"
       {...comments}
       value={comments.value || ''}
  />
</div>
@gaearon

This comment has been minimized.

Contributor

gaearon commented May 1, 2016

This is tangential but make sure you're testing performance with production version of Reacf.

@jhabdas

This comment has been minimized.

jhabdas commented May 1, 2016

@davidhtien try changing value to defaultValue to force the hand.

@davidhtien

This comment has been minimized.

davidhtien commented May 2, 2016

@jhabdas that fixed it! thanks man!

@jhabdas

This comment has been minimized.

jhabdas commented May 2, 2016

@davidhtien You're welcome. Please note holding a key will no longer cause repeated keystrokes to occur. But it's progress.

@erikras

This comment has been minimized.

Owner

erikras commented May 4, 2016

The performance of v6.0.0-alpha-7 looks really good, so I'm closing this issue.

@erikras erikras closed this May 4, 2016

@luisrudge

This comment has been minimized.

Contributor

luisrudge commented May 4, 2016

Thanks @erikras <3

@lock

This comment has been minimized.

lock bot commented Jun 3, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.