Improve block loading PHP performance #2252
Conversation
Gruntfile.js
Outdated
| @@ -1370,6 +1371,17 @@ module.exports = function(grunt) { | |||
| } | |||
| } ); | |||
|
|
|||
| grunt.registerTask( 'blockJson2PHP', 'Copies block.json file contents to block-json.php.', function() { | |||
There was a problem hiding this comment.
block.json files are copied with webpack:
wordpress-develop/tools/webpack/blocks.js
Lines 107 to 114 in 24b83dd
I think it would be better to convert them to PHP files in one go. At the moment, you would have both block.json and block-json.php files in the same folder where the former becomes unused.
There was a problem hiding this comment.
We could easily remove the JSON files, I'm just not sure if it's something we want to do or not.
- I was thinking that maybe we want to use the
.jsonfiles instead of the.phpfiles whenWP_DEBUGand/orSCRIPT_DEBUGistrue - The concept in this patch was to make this change as minimal as possible, avoiding any backwards-compatibility issues etc, just in case a plugin is accessing the JSON files for core blocks. Is that a valid worry, or do you think it's an edge-case? 🤔
Also, I'm not that familiar with webpack so using grunt was a crutch, the only way I knew how to do this 😅
|
How more performant does this become when we replace JSON files with PHP files? It's maybe 50-60 files to process. |
Hmmm I was looking for a way to run some PHP profiling when using |
|
Not exactly a concrete result, but it does confirm that there is indeed an improvement. add_action( 'init', function() {
$start = microtime( true );
for ( $i = 0; $i < 1000; $i++ ) {
register_block_type_from_metadata( '/var/www/src/wp-includes/blocks/button' );
register_block_type_from_metadata( '/var/www/src/wp-includes/blocks/buttons' );
register_block_type_from_metadata( '/var/www/src/wp-includes/blocks/cover' );
register_block_type_from_metadata( '/var/www/src/wp-includes/blocks/calendar' );
}
$end = microtime( true );
$diff = $end - $start;
echo 'Execution time: ' . $diff;
});The results of doing something like that are notoriously unreliable, so I kept refreshing my page for ~50 times before and after the patch, and kept the best result for both pre/post-patch. Prior to the patch, the best result I got was ~6.38s (max: 12) It's clear that the function now becomes faster (33% faster by my results), but I have no metric to see what the actual impact would be on a WP site... so I'll theorize a bit: |
|
Maybe we could find a way to combine all those PHP files for core into a single file if we are at 71 files already. It should further speed up the processing. You could write a file that returns key-value pairs where the path to the block directory is the key, and the metadata is the value. It would be more work, as you would have to check this array for core blocks but maybe it's worth it. |
|
Hmmm it would make sense... and we'd only have to load a single file. Great idea 👍 |
Applied the changes. It's marginally faster, but it makes more sense since we're only reading a single file instead of performing 70 filesystem requests. If the filesystem is relatively slow, then this should further improve the performance 👍 |
aristath
force-pushed
the
fix/55005
branch
2 times, most recently
from
61bbe6f to
688e7a5
Compare
|
It would be great to pretty print the block-json.php file so subsequent updates to the file produce a more readable diff, but it looks like json2php doesn't support pretty printing. |
There was a problem hiding this comment.
I like this initiative 🙌. I think it'd be really great to implement it a way that lets third parties do the same. For example, if register_block_type_from_metadata were modified to accept string|array for the first argument so the metadata could be passed in that way. I'm suggesting this because I've been working on build tooling for my own blocks and wanted do this same thing. I've made it work but I have to largely re-implement register_block_type_from_metatdata.
|
Thanks for the PR! I've attempted to rebase it to see if that would resolve the test failures, but it did not. It appears that there are two kinds of failures here:
|
|
I rebased too and also ran |
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
|
It looks like the latest change resolved all the issues. The failing CI check on Windows is a known issue that we are trying to address before WordPress 6.1 Beta 1. In the future, we could go even one step further and come up with a way to register multiple blocks at once by passing a similar array as in the |
| // Try to get metadata from the static cache for core blocks. | ||
| $metadata = false; | ||
| if ( str_starts_with( $file_or_folder, ABSPATH . WPINC ) ) { | ||
| $core_block_name = str_replace( ABSPATH . WPINC . '/blocks/', '', $file_or_folder ); |
There was a problem hiding this comment.
Have you considered using the same paths passed to the register_block_type as keys in the generated file? This way, you could do a simple check:
if ( ! empty( $core_blocks_meta[ $file_or_folder ] ) ) {
$metadata = $core_blocks_meta[ $file_or_folder ];
}There was a problem hiding this comment.
Aren't those paths absolute - which means they'd change from site to site? 🤔
There was a problem hiding this comment.
I was thinking you could set a variable to use in keys for the generated PHP file:
$dir = ABSPATH . WPINC . '/blocks/';Maybe, it's too much work in a bit different place 😄
|
@aristath I think it's good to set |
|
Thanks for the PR! Merged in r54276. |
|
I’m sure that @azaozz will be delighted when hee discovers that this optimization landed. Thank you so much for taking it to the finish line! |
|
I'm not seeing any hooks allowing devs to load their own
|
|
replied on your comment on https://make.wordpress.org/core/2022/10/07/improved-php-performance-for-core-blocks-registration/#comment-43894 to keep the discussion in a single place and not fragmented 👍 |
|
hey @aristath I saw that you added improvements to avoid loading the All we need is just a filter and everyone can create their own logic to load block data more efficiently. Changing the existing part ... if ( str_starts_with( $file_or_folder, ABSPATH . WPINC ) ) {
$core_block_name = str_replace( ABSPATH . WPINC . '/blocks/', '', $file_or_folder );
if ( ! empty( $core_blocks_meta[ $core_block_name ] ) ) {
$metadata = $core_blocks_meta[ $core_block_name ];
}
}... to this ... if ( str_starts_with( $file_or_folder, ABSPATH . WPINC ) ) {
$core_block_name = str_replace( ABSPATH . WPINC . '/blocks/', '', $file_or_folder );
if ( ! empty( $core_blocks_meta[ $core_block_name ] ) ) {
$metadata = $core_blocks_meta[ $core_block_name ];
}
} else {
$metadata = apply_filters( 'custom_blocks_meta', $metadata, $file_or_folder );
}... and that's all. Is this something that can be extended and should I open a new issue for it? |
This PR improves the PHP performance of blocks:
block.jsonfiles toblock-json.phpblock-json.phpfiles in theregister_block_type_from_metadata()functionNote: To test this patch, you'll need to run
npm run build, ornpm run grunt blockJson2PHPto generate the PHP files from JSON.Trac ticket: https://core.trac.wordpress.org/ticket/55005
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.