Parsing patterns when idling (performance follow-up for inserting patterns into containers) #29444
Conversation
|
Size Change: +159 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
david-szabo97
force-pushed
the
try/parse-patterns-when-idle
branch
from
6c43b60
to
ac25ca5
Compare
mtias
added
[Feature] Inserter
lib/block-patterns.php
Outdated
| @@ -62,3 +62,23 @@ | |||
| <!-- /wp:columns -->', | |||
| ) | |||
| ); | |||
|
|
|||
| for ( $i = 0; $i < 1000; $i++ ) { | |||
There was a problem hiding this comment.
For demonstration purposes only. I'll remove this once the PR is approved.
There was a problem hiding this comment.
Can we remove that code, in order to get accurate numbers in the job and judge whether the PR allows us to get the previous loading numbers?
| const marker = `[usePreParsePatterns] process ${ index } ${ patterns[ index ]?.title }`; | ||
| window.console.time( marker ); |
There was a problem hiding this comment.
window.console.time* are for demonstration purposes only. I'll remove this once the PR is approved.
david-szabo97
changed the title
david-szabo97
requested review from
ellatrix,
spacedmonkey and
TimothyBJacobs
as code owners
david-szabo97
requested review from
vindl,
jeyip,
mattwiebe,
youknowriad,
ntsekouras,
Copons,
Addison-Stavlo and
creativecoder
|
This is looking good for me but can we remove the debug code like #29444 (comment) to get a clear sense of the performance numbers on CI? |
|
It it's weird, the numbers in the job seem to indicate that this PR is actually a bit slower than trunk. That said, when looking at the original PR introducing this https://github.com/WordPress/gutenberg/runs/1855261445 the performance numbers are a bit slower than master but not that much. What was consistent for us though is that we started having lower loading time numbers after Gutenberg 10.0 in the release posts after that PR landed. Do you have any potential explanation for these numbers. |
|
Hey 👋 - thanks for working on this! I've started working with block patterns lately (#28891, #29602) and will start exploring I don't have a solution for what I'll suggest now, but wanted to share my initial thoughts. I understand that #28459 was implemented for a way to add patterns by checking the Since As I said I haven't explored this yet, but some solutions might be:
|
I don't think this is going to be an accurate comparison since we only have a handful of patterns. Parsing the current patterns takes about 100ms which is impossible to measure since we have some fluctuation as well.
We have an open issue with a few ideas: #28872 |
| const settings = useMemo( () => { | ||
| const _settings = { ...originalSettings }; | ||
| _settings.__experimentalBlockPatterns = []; | ||
| return _settings; | ||
| }, [ originalSettings ] ); |
There was a problem hiding this comment.
What's the reasoning behind this change?
There was a problem hiding this comment.
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 callback = () => { | ||
| index++; | ||
| if ( index >= patterns.length ) { | ||
| return; | ||
| } | ||
|
|
||
| select( blockEditorStore ).__experimentalGetParsedPattern( | ||
| patterns[ index ].name, | ||
| index | ||
| ); | ||
|
|
||
| handle = requestIdleCallback( callback ); | ||
| }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| select( blockEditorStore ).__experimentalGetParsedPattern( | ||
| patterns[ index ].name, | ||
| index |
There was a problem hiding this comment.
Is this second argument correct?
There was a problem hiding this comment.
No, that's not needed anymore. Thanks!
| const patterns = state.settings.__experimentalBlockPatterns; | ||
| return map( patterns, ( pattern ) => ( { | ||
| const pattern = patterns.find( ( { name } ) => name === patternName ); |
There was a problem hiding this comment.
You already have the pattern from __experimentalGetAllowedPatterns. No need to get state.settings.__experimentalBlockPatterns again and filter. Am I missing something?
There was a problem hiding this comment.
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.
|
@david-szabo97, there are some still open feedback points. What's the status of the PR? |
|
Missed the last comment. Answered it. Other than that there is nothing else to do. I'm happy to make any suggested changes if needed. |
Description
Related: #28872 (I don't think this PR fixes the issue. It's an improvement rather than a complete fix)
We need to parse all the patterns to determine whether a pattern can be inserted into the selected destination. We parse them immediately when the editor loads which freezes the editor until the parser finishes. To avoid the thread blocking, this PR changes it to only parse when we have resources to do so.
How has this been tested?
Screenshots
Before
Used 1000 dummy patterns for perf testing.
Parsing patterns blocks the editor for ~12 seconds. LCP at ~13500ms
After
Parsing patterns doesn't block the editor anymore. LCP at ~4200ms
Types of changes
Performance
Checklist: