Track the parent block to optimize hierarchy selectors by youknowriad · Pull Request #16392 · WordPress/gutenberg

youknowriad · 2019-07-02T12:11:23Z

Extracted from #16368

This PR tracks the parent of a block in the "blocks.parents" state tree in the reducer to optimizer the performance of some block editor selectors.

On its own, this PR doesn't seem to have a huge impact on performance bit it's a good step towards #16368

youknowriad · 2019-07-02T15:48:47Z

I fixed the memory leak in the parents reducer. It turns out that I discovered memory leaks in other reducers (byClientId and order), I won't be fixing those right now in this PR though. A generic fix would be good but it's not possible due to the fact that we track reusable blocks their as well.

youknowriad · 2019-07-03T10:06:30Z

I think this should be ready and I'm noticing something between 2-5% performance improvement.

aduth · 2019-07-03T18:28:32Z

+			case 'REMOVE_BLOCKS':
+				action = {
+					...action,
+					removedClientIds: getAllChildren( action.clientIds ),


Is there a specific reason we need to rename these properties from clientIds, rather than just "enhancing" the existing value to account for the cascade? Seems like it might make this a bit simpler to implement, since you wouldn't need the switch to vary the behavior.

I've been caught off guard in several occasions because of this. The fact that the property don't contain the same value between reducers (selection and blocks), and also the difference between the action creator (which I tend to take a look at to understand the action) led me to this change.

It is still a different action but instead but I think the fact that we introduce a new key makes it less confusing.

I get the motivation for this. It could, however, be pretty jarring for someone who notices the discrepancy and pulls up the definition of removeBlocks, since it clearly only returns clientIds in its payload.

Perhaps update removeBlocks to make that relationship clearer?

// actions.js yield { type: 'REMOVE_BLOCKS', clientIds, removedClientIds: undefined // Explain bla bla higher-order reducer };

In alternative, if we're really strict about action types, it would be more "correct" to let this higher-order reducer divert from one action type to a new one:

// reducer.js if ( state ) { switch ( action.type ) { case 'REMOVE_BLOCKS': action = { type: DERIVED_TYPE_SIMILAR_TO_REMOVE_BLOCKS, removedClientIds: …

Alternatively, with controls implementation, these days we could probably incorporate this into the action creator itself, rather than as a reducer enhancer. That way the logic is consolidated to one place.

aduth · 2019-07-03T18:29:31Z

+			case 'REPLACE_BLOCKS':
+				return {
+					...omit( state, action.replacedClientIds ),
+					...mapBlockParents( action.blocks, state[ action.clientIds[ 0 ] ] ),


Re: My previous comment, I guess we still reference the original clientIds here. Can you clarify what you're doing here in referencing only the first of the original of the clientIds ?

Basically we need the parent of the replaced blocks to assign it as parent of the inserted blocks. replacedClientIds contains more than just the "root" level which in theory could result in the wrong value here.

aduth · 2019-07-03T18:29:51Z

 			it( 'can replace a child block', () => {
 				const existingState = deepFreeze( {
 					byClientId: {
-						clicken: {


aduth · 2019-07-03T18:32:21Z

-		for ( let i = 0; i < clientIds.length; i++ ) {
-			clientIds.push( ...state.order[ clientIds[ i ] ] );
+		for ( let i = 0; i < result.length; i++ ) {
+			if ( state.order[ result[ i ] ] ) {


Is this condition related to your comment at #16392 (comment) with regards to orphaned references? Should we comment it as a temporary fix if it is meant to be temporary?

no, to be completely honest, I tried but failed to find why this condition is necessary. When you remove an "empty paragraph" using backspace, there's a JS error that is triggered if you remove this condition.

mcsf · 2019-07-04T12:51:44Z

+					...state,
+					[ action.clientId ]: action.toRootClientId || '',
+				};
+			}


Minor: unneeded block around return statement.

mcsf · 2019-07-04T13:03:58Z

+			case 'REMOVE_BLOCKS':
+				action = {
+					...action,
+					removedClientIds: getAllChildren( action.clientIds ),


I get the motivation for this. It could, however, be pretty jarring for someone who notices the discrepancy and pulls up the definition of removeBlocks, since it clearly only returns clientIds in its payload.

Perhaps update removeBlocks to make that relationship clearer?

// actions.js yield { type: 'REMOVE_BLOCKS', clientIds, removedClientIds: undefined // Explain bla bla higher-order reducer };

In alternative, if we're really strict about action types, it would be more "correct" to let this higher-order reducer divert from one action type to a new one:

// reducer.js if ( state ) { switch ( action.type ) { case 'REMOVE_BLOCKS': action = { type: DERIVED_TYPE_SIMILAR_TO_REMOVE_BLOCKS, removedClientIds: …

aduth

I'm not sure if you wanted to make revisions based on @mcsf 's comment (I think either could maybe help), but in any case this looks like a solid improvement as it is.

aduth · 2019-07-05T12:59:30Z

 				},
 			};

-			expect( getBlockHierarchyRootClientId( state, 56 ) ).toBe( '123' );


Considering it's documented to return a value of type string, I feel like we should choose clientIds which allow us to test for this to be true.

gutenberg/packages/block-editor/src/store/selectors.js

Lines 484 to 486 in 21e2dce

* @return {string} Root client ID

*/

export const getBlockHierarchyRootClientId = createSelector(

aduth · 2019-07-05T13:02:09Z

+			case 'REMOVE_BLOCKS':
+				action = {
+					...action,
+					removedClientIds: getAllChildren( action.clientIds ),


Alternatively, with controls implementation, these days we could probably incorporate this into the action creator itself, rather than as a reducer enhancer. That way the logic is consolidated to one place.

…rnmobile/track-unsupported-blocks * 'master' of https://github.com/WordPress/gutenberg: Bump plugin version to 6.1.0-rc.1 Update HTML anchor explaination text (#16142) Move post permalink to beneath title on mobile. (#16277) Export cloneBlock method to the mobile (#16441) Fix inconsistent references to Settings Sidebar (#16138) Adds a cache key to the blocks reducer in order to optimize the getBlock selector (#16407) Track the parent block to optimize hierarchy selectors (#16392)

Track the parent block to optimize hierarchy selectors

f1726fb

youknowriad added the [Type] Performance Related to performance efforts label Jul 2, 2019

youknowriad requested review from ellatrix, gziolo and talldan as code owners July 2, 2019 12:11

youknowriad self-assigned this Jul 2, 2019

youknowriad requested a review from aduth July 2, 2019 12:11

aduth reviewed Jul 2, 2019

View reviewed changes

Comment thread packages/block-editor/src/store/reducer.js Outdated

Comment thread packages/block-editor/src/store/reducer.js Outdated

Comment thread packages/block-editor/src/store/reducer.js Outdated

youknowriad added 4 commits July 2, 2019 13:52

Fix unit tests

2d3ea3a

Changes per review

705d0fd

Avoid memory leaks in the parent reducer

afefd02

Add unit tests for the parents reducer

b21c2bf

Fix all memory leaks

9d97b1b

youknowriad mentioned this pull request Jul 3, 2019

Adds a cache key to the blocks reducer in order to optimize the getBlock selector #16407

Merged

aduth reviewed Jul 3, 2019

View reviewed changes

Changes per review

e05ab9e

youknowriad mentioned this pull request Jul 4, 2019

WIP: optimize get block selector #16368

Closed

mcsf reviewed Jul 4, 2019

View reviewed changes

aduth approved these changes Jul 5, 2019

View reviewed changes

youknowriad added 2 commits July 5, 2019 16:49

Change action type for clarity

6087e69

Use string keys in unit test

3002180

youknowriad merged commit ab847cb into master Jul 5, 2019

github-actions Bot added this to the Gutenberg 6.1 milestone Jul 5, 2019

aduth deleted the add/parent-block-reducer branch July 10, 2019 12:21

	* @return {string} Root client ID
	*/
	export const getBlockHierarchyRootClientId = createSelector(

Conversation

youknowriad commented Jul 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

youknowriad commented Jul 2, 2019

Uh oh!

youknowriad commented Jul 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants