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.

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

Parsing patterns when idling (performance follow-up for inserting patterns into containers) #29444

Merged
merged 19 commits into from
Mar 24, 2021
Merged

Parsing patterns when idling (performance follow-up for inserting patterns into containers) #29444

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c0edc39
Try parsing patterns when idling
david-szabo97 Mar 1, 2021
4298360
Increase number of patterns
david-szabo97 Mar 2, 2021
c1d85b4
Limit to a single task per idle callback
david-szabo97 Mar 2, 2021
b0b6b43
Update docs
david-szabo97 Mar 2, 2021
39bc7eb
Use requestIdleCallback instead of queue
david-szabo97 Mar 2, 2021
34e4aad
Revert queue
david-szabo97 Mar 2, 2021
ac25ca5
Refactor
david-szabo97 Mar 2, 2021
0e0e2b5
Fix errors
david-szabo97 Mar 4, 2021
4df0a4e
requestIdleCallback and cancelIdleCallback fallback
david-szabo97 Mar 4, 2021
8f07730
Refactor to use getParsedPattern
david-szabo97 Mar 5, 2021
9a200cd
Fix tests
david-szabo97 Mar 5, 2021
a8b3d46
Fix hook rerunning on render
david-szabo97 Mar 9, 2021
b12d4d6
Move hook call into BlockList
david-szabo97 Mar 9, 2021
2945982
Fix deps
david-szabo97 Mar 9, 2021
eb4e1e0
Remove patterns from the block preview context
david-szabo97 Mar 9, 2021
f402bd5
Memoize modified settings
david-szabo97 Mar 9, 2021
fb07d35
Remove debug code snippets
david-szabo97 Mar 10, 2021
1a176d2
Remove unused argument
david-szabo97 Mar 11, 2021
cac8f34
Merge branch 'trunk' into try/parse-patterns-when-idle
david-szabo97 Mar 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/block-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import useInsertionPoint from './insertion-point';
import BlockPopover from './block-popover';
import { store as blockEditorStore } from '../../store';
import { useScrollSelectionIntoView } from '../selection-scroll-into-view';
import { usePreParsePatterns } from '../../utils/pre-parse-patterns';

export const BlockNodes = createContext();
export const SetBlockNodes = createContext();
Expand All @@ -28,6 +29,7 @@ export default function BlockList( { className } ) {
const [ blockNodes, setBlockNodes ] = useState( {} );
const insertionPoint = useInsertionPoint( ref );
useScrollSelectionIntoView( ref );
usePreParsePatterns();

return (
<BlockNodes.Provider value={ blockNodes }>
Expand Down
12 changes: 8 additions & 4 deletions packages/block-editor/src/components/block-patterns-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
/**
* WordPress dependencies
*/
import { useMemo } from '@wordpress/element';
import { parse } from '@wordpress/blocks';
import {
VisuallyHidden,
__unstableComposite as Composite,
Expand All @@ -11,16 +9,22 @@ import {
} from '@wordpress/components';
import { useInstanceId } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';
import { useSelect } from '@wordpress/data';

/**
* Internal dependencies
*/
import BlockPreview from '../block-preview';
import InserterDraggableBlocks from '../inserter-draggable-blocks';
import { store as blockEditorStore } from '../../store';

function BlockPattern( { isDraggable, pattern, onClick, composite } ) {
const { content, viewportWidth } = pattern;
const blocks = useMemo( () => parse( content ), [ content ] );
const { name, viewportWidth } = pattern;
const { blocks } = useSelect(
( select ) =>
select( blockEditorStore ).__experimentalGetParsedPattern( name ),
[ name ]
);
const instanceId = useInstanceId( BlockPattern );
const descriptionId = `block-editor-block-patterns-list__item-description-${ instanceId }`;

Expand Down
7 changes: 6 additions & 1 deletion packages/block-editor/src/components/block-preview/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,15 @@ export function BlockPreview( {
__experimentalLive = false,
__experimentalOnClick,
} ) {
const settings = useSelect(
const originalSettings = useSelect(
( select ) => select( blockEditorStore ).getSettings(),
[]
);
const settings = useMemo( () => {
const _settings = { ...originalSettings };
_settings.__experimentalBlockPatterns = [];
return _settings;
}, [ originalSettings ] );
Comment on lines +31 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning behind this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't want to provide patterns in block previews. Since we use the usePreParsePatterns in the BlockList component, it would start parsing patterns for every block preview.

const renderedBlocks = useMemo( () => castArray( blocks ), [ blocks ] );
if ( ! blocks || blocks.length === 0 ) {
return null;
Expand Down
30 changes: 18 additions & 12 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
filter,
mapKeys,
orderBy,
every,
} from 'lodash';
import createSelector from 'rememo';

Expand Down Expand Up @@ -1757,13 +1756,18 @@ export const __experimentalGetAllowedBlocks = createSelector(
]
);

const __experimentalGetParsedPatterns = createSelector(
( state ) => {
export const __experimentalGetParsedPattern = createSelector(
( state, patternName ) => {
const patterns = state.settings.__experimentalBlockPatterns;
return map( patterns, ( pattern ) => ( {
const pattern = patterns.find( ( { name } ) => name === patternName );
Copy link
Contributor

Choose a reason for hiding this comment

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

You already have the pattern from __experimentalGetAllowedPatterns. No need to get state.settings.__experimentalBlockPatterns again and filter. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal is to have a selector function that can be memoized easily. The pattern name as parameter is immutable. If we used the pattern object then it might happen that we provide a modified/reconstructed pattern object that has a different reference.

if ( ! pattern ) {
return null;
}

return {
...pattern,
contentBlocks: parse( pattern.content ),
} ) );
blocks: parse( pattern.content ),
};
},
( state ) => [ state.settings.__experimentalBlockPatterns ]
);
Expand All @@ -1778,17 +1782,19 @@ const __experimentalGetParsedPatterns = createSelector(
*/
export const __experimentalGetAllowedPatterns = createSelector(
( state, rootClientId = null ) => {
const patterns = __experimentalGetParsedPatterns( state );

const patterns = state.settings.__experimentalBlockPatterns;
if ( ! rootClientId ) {
return patterns;
}

const patternsAllowed = filter( patterns, ( { contentBlocks } ) => {
return every( contentBlocks, ( { name } ) =>
const parsedPatterns = patterns.map( ( { name } ) =>
__experimentalGetParsedPattern( state, name )
);
const patternsAllowed = filter( parsedPatterns, ( { blocks } ) =>
blocks.every( ( { name } ) =>
canInsertBlockType( state, name, rootClientId )
);
} );
)
);

return patternsAllowed;
},
Expand Down
4 changes: 4 additions & 0 deletions packages/block-editor/src/store/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3375,10 +3375,12 @@ describe( 'selectors', () => {
settings: {
__experimentalBlockPatterns: [
{
name: 'pattern-a',
title: 'pattern with a',
content: `<!-- wp:test-block-a --><!-- /wp:test-block-a -->`,
},
{
name: 'pattern-b',
title: 'pattern with b',
content:
'<!-- wp:test-block-b --><!-- /wp:test-block-b -->',
Expand Down Expand Up @@ -3409,10 +3411,12 @@ describe( 'selectors', () => {
settings: {
__experimentalBlockPatterns: [
{
name: 'pattern-a',
title: 'pattern a',
scope: { block: [ 'test/block-a' ] },
},
{
name: 'pattern-b',
title: 'pattern b',
scope: { block: [ 'test/block-b' ] },
},
Expand Down
65 changes: 65 additions & 0 deletions packages/block-editor/src/utils/pre-parse-patterns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/**
* WordPress dependencies
*/
import { useSelect, select } from '@wordpress/data';
import { useEffect } from '@wordpress/element';

/**
* Internal dependencies
*/
import { store as blockEditorStore } from '../store';

const requestIdleCallback = ( () => {
if ( typeof window === 'undefined' ) {
return ( callback ) => {
setTimeout( () => callback( Date.now() ), 0 );
};
}

return window.requestIdleCallback || window.requestAnimationFrame;
} )();

const cancelIdleCallback = ( () => {
if ( typeof window === 'undefined' ) {
return clearTimeout;
}

return window.cancelIdleCallback || window.cancelAnimationFrame;
} )();

export function usePreParsePatterns() {
const patterns = useSelect(
( _select ) =>
_select( blockEditorStore ).getSettings()
.__experimentalBlockPatterns,
[]
);

useEffect( () => {
if ( ! patterns?.length ) {
return;
}

let handle;
let index = -1;

const callback = () => {
index++;
if ( index >= patterns.length ) {
return;
}

select( blockEditorStore ).__experimentalGetParsedPattern(
patterns[ index ].name,
index
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this second argument correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that's not needed anymore. Thanks!

);

handle = requestIdleCallback( callback );
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you compared performance between different batching strategies? Roughly:

const patternsPerBatch = 5;

// ...

const callback = () => {
  index += patternsPerBatch;
  if ( index >= patterns.length ) return;

  for (let i = 0; i < patternsPerBatch; i++) {
    select( blockEditorStore ).__experimentalGetParsedPattern(
      patterns[ index + i ].name
    );
  }

I suggest trying this out with a few different values for patternsPerBatch. In order for the benchmark to mean something, the dummy patterns would have to resemble actual patterns in production — the idea is to determine whether the overhead of setting up requestIdleCallback is significant when parsing very small sets of patterns.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have tested this. We don't know how long a pattern takes to be parsed. After all, one pattern per batch worked out the best. Anything else usually took more time than it should.


handle = requestIdleCallback( callback );
return () => cancelIdleCallback( handle );
}, [ patterns ] );

return null;
}
4 changes: 0 additions & 4 deletions packages/edit-post/src/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,6 @@ function Editor( {
const isFSETheme = getEditorSettings().isFSETheme;
const isViewable = getPostType( postType )?.viewable ?? false;

// Prefetch and parse patterns. This ensures patterns are loaded and parsed when
// the editor is loaded rather than degrading the performance of the inserter.
select( 'core/block-editor' ).__experimentalGetAllowedPatterns();

return {
hasFixedToolbar:
isFeatureActive( 'fixedToolbar' ) ||
Expand Down
4 changes: 0 additions & 4 deletions packages/edit-site/src/components/editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,6 @@ function Editor( { initialSettings } ) {
const postType = getEditedPostType();
const postId = getEditedPostId();

// Prefetch and parse patterns. This ensures patterns are loaded and parsed when
// the editor is loaded rather than degrading the performance of the inserter.
select( 'core/block-editor' ).__experimentalGetAllowedPatterns();

// The currently selected entity to display. Typically template or template part.
return {
isInserterOpen: isInserterOpened(),
Expand Down