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

Performance: Memoize multi selection selectors #3170

Merged
merged 1 commit into from
Oct 26, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 26, 2017

This pull request is a first pass at render performance for posts containing many blocks. I had noted that simple changes to editor state were causing the entire list of blocks to rerender, which could degrade performance particularly when many blocks exist. With the default behavior of react-redux's connect applying "pure" behavior, it was unexpected that other components should be rerendering aside from the one being hovered.

Implementation notes:

In this case, the VisualBlockList component would rerender because the multi-selection selectors it depends on would always map to return a new instance of an array. This would cascade down to each individual block because we assign a new dynamic ref function reference in the VisualBlockList component. This latter behavior is similarly non-ideal, but not as simple to resolve, so will be addressed separately.

Testing instructions:

Verify there are no regressions in the behavior of block multi-selection.

Ensure unit tests pass:

npm run test-unit

If you have React Developer Tools extension installed, toggle "Highlight Updates" and verify hovering only impacts relevant block (also layout, which will be debugged separately).

@codecov
Copy link

codecov bot commented Oct 26, 2017

Codecov Report

Merging #3170 into master will increase coverage by 0.02%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3170      +/-   ##
==========================================
+ Coverage   31.61%   31.64%   +0.02%     
==========================================
  Files         217      217              
  Lines        6278     6282       +4     
  Branches     1116     1116              
==========================================
+ Hits         1985     1988       +3     
- Misses       3609     3610       +1     
  Partials      684      684
Impacted Files Coverage Δ
editor/selectors.js 95.21% <85.71%> (-0.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2e25cd...2f672df. Read the comment docs.

@mtias
Copy link
Member

mtias commented Oct 26, 2017

Testing looks good to me.

@aduth aduth force-pushed the update/perf-render-selectors branch from 38d2690 to 2f672df Compare October 26, 2017 18:23
@aduth aduth merged commit 4dd768d into master Oct 26, 2017
@aduth aduth deleted the update/perf-render-selectors branch October 26, 2017 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Priority] High Used to indicate top priority items that need quick attention [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants