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

perf(getUnhandledProps): replace lodash with vanilla js #860

Merged
merged 1 commit into from
Nov 15, 2016

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Nov 15, 2016

Fixes #859

TL;DR Conclusion at the bottom.

As with the linked issue, I've also noticed some severe performance issues, mostly in Grid and Grid column. It appears there is a memory leak somewhere in getUnhandledProps due to lodash.

Calculating unhandled props can be expensive due to the several nested object references it has to traverse to first calculate the handled props: autoControlledProps, defaultProps keys, props keys. It then needs to loop the props again to omit all those keys. This can be done several times per render, per element. This alone should not be responsible for the results we're seeing below, however.

This PR replaces lodash with vanilla JS, resulting in >12,000 times performance improvement .

It also caches handledProps so they are only calculated once per component (handled props never change). This will be fixed permanently once #731 is merged as we'll have a static array of handled props baked into every component.

Rough Perf Analysis

No cache, lodash (baseline)

After lots of testing, I notice that the Grid and Grid column render times seem increase every render for the particular view I am testing against. Note the instance count and render count remains constant, but the time required is growing rapidly.

First Render - 1s

image

Second Render - 3s

image

Third Render - 27s

image

With cache, lodash

First Render - 1.5s

image

Second Render - 21s

image

Third Render - 1.3s

image

Additional tests...

I found the 21s odd here, so I ran renders 4 and 5 with results of 3.6s and 2.8s. This seems odd.

No cache, vanilla JS

All three runs completed in <=2ms (approx.) 😮. This certainly means, lodash is at fault here.

image

Cache and vanilla JS

Using vanilla JS is obviously sufficient, however, I tested vanilla with cache for completeness. The results were not significantly different from vanilla JS without cache.

Conclusion

Something in lodash is extremely non-performant given the usage in our getUnhandledProps() lib method. Using vanilla JS increases performance by up to >12,000 times. Caching makes no difference and is not necessary.

@levithomason levithomason changed the title perf(getUnhandledProps): add handledProps cache perf(getUnhandledProps): replace lodash with vanilla js Nov 15, 2016
@levithomason levithomason force-pushed the perf/get-unhandled-props branch from f0452d6 to 70a3a43 Compare November 15, 2016 19:38
@codecov-io
Copy link

codecov-io commented Nov 15, 2016

Current coverage is 99.95% (diff: 100%)

Merging #860 into master will decrease coverage by 0.04%

@@           master       #860   diff @@
========================================
  Files         137        137          
  Lines        2254       2254          
  Methods         0          0          
  Messages        0          0          
  Branches        0          0          
========================================
- Hits         2254       2253     -1   
- Misses          0          1     +1   
  Partials        0          0          

Powered by Codecov. Last update cf14691...70a3a43

@levithomason levithomason force-pushed the perf/get-unhandled-props branch from 70a3a43 to 2c50aea Compare November 15, 2016 19:51
@levithomason levithomason merged commit 1036fb1 into master Nov 15, 2016
@levithomason levithomason deleted the perf/get-unhandled-props branch November 15, 2016 19:59
@levithomason
Copy link
Member Author

Released in [email protected].

@markerikson
Copy link

Hah. I'm playing around with Semantic-UI-React on a sample project for the first time, and just ran into this behavior myself (in my case, with a tabbed menu acting as parent to some panels with a couple simple forms). After adding a table and a small form,the time to switch tabs became extremely noticeable. A perf profile showed the issue you found here, and fortunately I ran a quick search before filing another issue :)

markerikson added a commit to markerikson/project-minimek that referenced this pull request Nov 22, 2016
Important note: Semantic-UI-React 0.60.9 had a major perf issue with
how Lodash was being used to omit props. This was resolved in 0.60.10,
so future installs should be okay.

References:
Semantic-Org/Semantic-UI-React#860
Semantic-Org/Semantic-UI-React#864
markerikson added a commit to markerikson/project-minimek that referenced this pull request Nov 22, 2016
I was previously on 0.60.9.  Unfortunately, that apparently had a huge
performance issue, due to the way Lodash was being used to omit props
that components didn't handle themselves.  That resulted in increasingly
slow performance to switch tabs as I added more components per tab
panel (and not that many components, either!).  The perf issue was
fixed in 0.60.10, naturally.

References:

Semantic-Org/Semantic-UI-React#860
Semantic-Org/Semantic-UI-React#864
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.

Very low performance caused by getUnhandledProps
3 participants