Skip to content

Performance: Use Lodash flatMap for accumulating blocks in block list #3185

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 26, 2017
Merged

Performance: Use Lodash flatMap for accumulating blocks in block list #3185

merged 1 commit into from
Oct 26, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 26, 2017 •
edited
Loading

This pull request seeks to optimize rendering of block list block elements by using Lodash's flatMap instead of accumulating an array of flattened blocks with a combination of Array#reduce and Array#concat. Benchmarks reveal that this performs approximately 4x faster when run on my machine:

https://jsperf.com/lodash-flatmap-vs-array-reduce/1

Further, the revised implementation is more concise.

h/t @ockham for pointer to this Lodash utility I've yet overlooked 😄 (see Automattic/wp-calypso#19105)

Testing instructions:

Verify that there are no regressions in the rendering of block list, particularly in sibling inserters between blocks.

@aduth aduth added the [Type] Performance Related to performance efforts label Oct 26, 2017
@codecov
Copy link

codecov bot commented Oct 26, 2017 •
edited
Loading

Codecov Report

Merging #3185 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3185   +/-   ##
=======================================
  Coverage   31.61%   31.61%           
=======================================
  Files         217      217           
  Lines        6278     6278           
  Branches     1116     1116           
=======================================
  Hits         1985     1985           
  Misses       3609     3609           
  Partials      684      684
Impacted Files Coverage Δ
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️

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 1b444cb...670a72f. Read the comment docs.

@ephox-mogran
Copy link
Contributor

I get similar performance benefits. I assume (looking at the code), it's because the baseFlatten that is called internally in flatMap is mutating the array directly (rather than the concat which is both non-destructive and called several times in reduce). Is that your understanding?

@aduth
Copy link
Member Author

aduth commented Oct 26, 2017

Yes, also because Lodash is not fully spec-compliant about sparse arrays which tends to lead to a performance win over native equivalents:

https://stackoverflow.com/a/18884620

@ockham
Copy link
Contributor

ockham commented Oct 26, 2017

Hmm, there goes my chance for an easy first Gutenberg patch 😄

@aduth aduth merged commit aa13720 into master Oct 26, 2017
@aduth aduth deleted the update/perf-block-list-flat-map branch October 26, 2017 20:52
@aduth aduth mentioned this pull request Oct 30, 2017
Merged
@mtias
Copy link
Member

mtias commented Oct 30, 2017

This is really nice; thanks @aduth and @ockham.

@aduth aduth mentioned this pull request Jan 4, 2018
Merged
@mcsf mcsf mentioned this pull request Jan 17, 2018
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
[Type] Performance Related to performance efforts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aduth @ephox-mogran @ockham @mtias