-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[RNMobile] performance improvements #27446
Conversation
Size Change: 0 B Total Size: 1.36 MB ℹ️ View Unchanged
|
Hello @dratwas, during testing I've discovered a regression on your branch, please look on it:
|
Thank you @lukewalczak for reporting this. I fixed it, could you please try it with the latest changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM! Also the previously mentioned issue is fixed. I have tested touched blocks on both platforms. Thanks @dratwas!
Description
In this PR I added some performance fixes that I found during my performance audit.
gutenberg-mobile PR wordpress-mobile/gutenberg-mobile#3088
The
rootBlockId
was declared after theblockCount
so we always used undefined as an argument ingetBlockCount
https://github.com/WordPress/gutenberg/pull/27446/files#diff-753322c95666679508eecb52371e38647190768c3198f16c1dc312a57c5b0e41R346
The block variation picker is rendered each time something changes in the store, thanks to the
useMemo
we avoid unnecessary re-rendershttps://github.com/WordPress/gutenberg/pull/27446/files#diff-c5fca3838ebdbf49b8a75d83c2da3706e812c3fe400c36bf0bbfe3f0c6477145R76
The
isAnyBlockSelected
should be boolean but sometimes it is a block because of this assignmenthttps://github.com/WordPress/gutenberg/pull/27446/files#diff-c3e8e7a2a2d154022e4803ff7007fdedda65e535389a9dbbfa320e037cbfe4dcR293
I just cast it to the boolean and thanks to that we avoid unnecessary renders
https://github.com/WordPress/gutenberg/pull/27446/files#diff-c3e8e7a2a2d154022e4803ff7007fdedda65e535389a9dbbfa320e037cbfe4dcR351
The button block uses a
selectedId
as a prop which forces the render of every button when we change selection.It was not only re-render but also an execution of
onToggleButtonFocus
which callsetState
inside.https://github.com/WordPress/gutenberg/pull/27446/files#diff-982d4f7ade1829603ee66453ddd8d0aa0a8fa416b82917911d07a7fb924695b6R75
I found that we need only info about that if specific button
isSelected
so i removed theselectedId
and use theisSelected
insteadhttps://github.com/WordPress/gutenberg/pull/27446/files#diff-982d4f7ade1829603ee66453ddd8d0aa0a8fa416b82917911d07a7fb924695b6R160
I also added a check to prevent of calling
setState
when the value is the samehttps://github.com/WordPress/gutenberg/pull/27446/files#diff-982d4f7ade1829603ee66453ddd8d0aa0a8fa416b82917911d07a7fb924695b6R160
The
isEditorSidebarOpened
changes each time we open/close the block settings bottom sheet. If we use it as a prop then it forces re-render even if we don't need to (the block is not selected). I added a check to use theisEditorSidebarOpened
only if block is selectedhttps://github.com/WordPress/gutenberg/pull/27446/files#diff-982d4f7ade1829603ee66453ddd8d0aa0a8fa416b82917911d07a7fb924695b6R430
I moved the
layoutProp
to const to not generate new object on every renderhttps://github.com/WordPress/gutenberg/pull/27446/files#diff-4347ded500d1f93237e9e254af2e2f5a01896b5d1736c61f965f6d333847f8bdR27
I also used
useCallback
to avoid of passing new function reference to childrenhttps://github.com/WordPress/gutenberg/pull/27446/files#diff-4347ded500d1f93237e9e254af2e2f5a01896b5d1736c61f965f6d333847f8bdR79
https://github.com/WordPress/gutenberg/pull/27446/files#diff-4347ded500d1f93237e9e254af2e2f5a01896b5d1736c61f965f6d333847f8bdR114
https://github.com/WordPress/gutenberg/pull/27446/files#diff-9c40c4e10ac9a0506fba110cc380f3ffa8debc0b2ab65fe6501c9615e0169c3aR453
I did the same with file block as in case of button block. I added check if block is selected to the
isEditorSidebarOpened
https://github.com/WordPress/gutenberg/pull/27446/files#diff-6a1044ed83415e71b054aaebbee6a525cd1c95f0f45b8328312378c842e57f30R583
I removed not used props in the media-text block
https://github.com/WordPress/gutenberg/pull/27446/files#diff-61739aa4e859ae0d5534c4b2caf2bce5df830b32585a2d6e6aa54ccca43fa225L395
I added
useCallback
to the paragraph block to avoid of creating new references on every renderhttps://github.com/WordPress/gutenberg/pull/27446/files#diff-61739aa4e859ae0d5534c4b2caf2bce5df830b32585a2d6e6aa54ccca43fa225L395
The link picker called
setAttributes
even if the attributes were the same which forced re-render of all providers etc. I added some check to avoid ithttps://github.com/WordPress/gutenberg/pull/27446/files#diff-1c6b716518b93eef13a216ed21fa8d92122f8941fbac8378f13f73a4ae5314dcR152
I added a check in the
layout.js
to avoid setting state with the same valuehttps://github.com/WordPress/gutenberg/pull/27446/files#diff-f687f5c0996917c65cac644a7e5bd220c61f4d81fb9dc2449fe522cf8bb33c1bR83
I added a babel plugin that will remove all
console[logs/warns]
in the production bundleshttps://github.com/WordPress/gutenberg/pull/27446/files#diff-6838f0ad1bf0f202fcb60582f166bd8dc57a53388f9a194b85c5c068b4bd6906R57
I tested it and seems like something has changed because the
onSelectionchange
is called when we select the paragraph and this comment is not valid. The selected paragraph was rencered two times because we called theonSelectionChange
with 0/0 or old values and then with the correct value (from Aztec). I removed this code since i couldn't find a scenario when it would be needed. Thanks to that we do not dispatch 2 actions but only one.https://github.com/WordPress/gutenberg/pull/27446/files#diff-7af18df9c3e07e67518894550a2e1d9d627feb879a2003a2e231ef927d4d06f8L704
I believe that this is the most important change but it is really small. Please note that:
NEVER do something like this in the useSelect
Because of that, every rich-text is rendered on each change in the redux-store. Imagine that we have a post with 100 rich text and all of them are re-rendered if we type something in one of them. And it is just because of
|| {}
:)https://github.com/WordPress/gutenberg/pull/27446/files#diff-7af18df9c3e07e67518894550a2e1d9d627feb879a2003a2e231ef927d4d06f8L704
How has this been tested?
The app should run a bit faster - easier to observe with large posts
Types of changes
I added some performance fixes
Checklist: