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

Add hasInserterItems selector and improve getInserterItems selector performance #11845

Merged
merged 1 commit into from
Nov 15, 2018

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Nov 14, 2018

The getInserterItems is called often (even if the inserter in not open) and its memoization is not performant.

In this PR, I'm adding a new hasInserterItems selector which is more performant as it doesn't have to create inserter items and can return quickly after one insertable item is found.

I also refactored a little bit the selectors to avoid using a memoized version of canInsertBlockType as I think it's just not efficient to call memoized selectors inside other memoized selectors.

I don't have the numbers as it's very hard to measure but I'd love your thoughts. I'm also considering dropping the memoization of these two selectors entirely because the cache is invalidated too often IMO.

@youknowriad youknowriad added the [Type] Performance Related to performance efforts label Nov 14, 2018
@youknowriad youknowriad self-assigned this Nov 14, 2018
@youknowriad youknowriad requested review from noisysocks and a team November 14, 2018 09:30
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explanation makes sense to me and I think the code reads fine. 👍

packages/editor/src/store/selectors.js Show resolved Hide resolved
@aduth
Copy link
Member

aduth commented Nov 14, 2018

Anecdotally, I observed in a recent performance audit that this function (getInserterItems) accounted for 20% of total JavaScript time spent while actively typing within a Columns block content:

image

(Which is to say: This pull request should hopefully help)

@youknowriad youknowriad force-pushed the try/improve-inserter-performance branch from 726d03a to a4137cc Compare November 15, 2018 08:43
@youknowriad youknowriad merged commit 4a54cc1 into master Nov 15, 2018
@youknowriad youknowriad deleted the try/improve-inserter-performance branch November 15, 2018 09:39
@lkraav
Copy link

lkraav commented Nov 15, 2018

I built and ran this minute's master 4a54cc1 on our sample problematic content, and was not able to detect any user-visible block editing performance improvement yet. Quietly assuming here it requires more pieces to fall in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants