Skip to content

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

Merged
aduth merged 1 commit intomasterfrom
update/perf-block-list-flat-map
Oct 26, 2017
Merged

Performance: Use Lodash flatMap for accumulating blocks in block list#3185
aduth merged 1 commit intomasterfrom
update/perf-block-list-flat-map

Conversation

@aduth
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor

ephox-mogran commented Oct 26, 2017

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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