Lazy Loading block-patterns - Trac 59532 #5941
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
| @@ -213,6 +222,25 @@ public function get_all_registered( $outside_init_only = false ) { | |||
| ); | |||
| $hooked_blocks = get_hooked_blocks(); | |||
| foreach ( $patterns as $index => $pattern ) { | |||
| if ( ! isset( $pattern['name'] ) ) { | |||
There was a problem hiding this comment.
I don't feel this is a neat code - I would appreciate suggestions and help here.
After the current changes to _register_theme_block_patterns, $patterns had many copies of patterns with blank content and no other properties like bellow
[7] => Array
(
[content] =>
)
[8] => Array
(
[content] =>
)I feel the issue was with the removal of continue; However, I could not find out how it affects names and where the origin of blank content is.
This if is more of a temporary fix to get the old test cases running, we need to get to the origin of this issue.
There was a problem hiding this comment.
This sounds like a strange bug to me. Previously, if the content was loaded from the file and the result was no content, the pattern would not be registered at all. By lazily reading that file data, we'll need to remove these when they are encountered during getters. Can you try to share how to reproduce the case you're running into so we can see if additional changes to the register() method are needed to validate a pattern before it is added to the registry? That way the first block would become unnecessary.
There was a problem hiding this comment.
to reproduce error->
comment this block and run
docker-compose run php ./vendor/bin/phpunit --filter test_register_theme_block_patterns_on_init
The error gets removed if we revert back code from
[src/wp-includes/block-patterns.php](https://github.com/WordPress/wordpress-develop/pull/5941/files#diff-b4514aa0e877e5200774c5cd82ec0870446f0a93ec7699eaa43c1a33c0d4f74a)
kt-12
changed the title
|
We tried getting values if we cache The initial result shows caching has slowed down the result, but I feel it was because of the use of serialize on a large array. |
| @@ -213,6 +222,25 @@ public function get_all_registered( $outside_init_only = false ) { | |||
| ); | |||
| $hooked_blocks = get_hooked_blocks(); | |||
| foreach ( $patterns as $index => $pattern ) { | |||
| if ( ! isset( $pattern['name'] ) ) { | |||
There was a problem hiding this comment.
This sounds like a strange bug to me. Previously, if the content was loaded from the file and the result was no content, the pattern would not be registered at all. By lazily reading that file data, we'll need to remove these when they are encountered during getters. Can you try to share how to reproduce the case you're running into so we can see if additional changes to the register() method are needed to validate a pattern before it is added to the registry? That way the first block would become unnecessary.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@joemcgill I have made a slight code refactor here after your last review. Instead of |
Co-authored-by: Joe McGill <801097+joemcgill@users.noreply.github.com>
|
Merged in https://core.trac.wordpress.org/changeset/57683. Thanks @kt-12. If you had additional tests you wanted to add, we can do those as follow-up commits, but I don't think they are necessary given that the public API hasn't changed and the existing tests cover these changes. |
|
@joemcgill I have added 2 unit tests. Couldn't test it earlier due to mysql docker issue. Also, I had to open a |
This is an enhancement to original PR - #5398, to adapt to the new changes in the trunk and fix some BC issues.
The idea is to load the pattern file's content only when the pattern is required.
Trac ticket: https://core.trac.wordpress.org/ticket/59532
Performance Improvement
We do see an improvement of
~5 msfor TT4. This improvement mainly comes from runninginclude $files_pathonly when we need the content. In terms of lazy loading, I feel this is the best we can do for now as the function is optimised at the registration end and content parsing of blocks is already happening in a JIT fashion.One improvement we can make is caching the output of parsed content along with caching
$file_pathsmodification timestats( $file_path )['mtime']. This might be a problem for a system using multiple copies of servers (like in Kubernetes), as each node will have a different file timestamp making it hard to cache. To avoid that we can usefilesizefrom stats instead with an assumption that any changes made to the file will change the file by at least a byte, which can be combined with the expiry time.There is not much change observed for TT3.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.