-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
block.json
files are copied with webpack:
wordpress-develop/tools/webpack/blocks.js
Lines 107 to 114 in 24b83dd
const blockMetadataFiles = { | |
'widgets/src/blocks/legacy-widget/block.json': 'wp-includes/blocks/legacy-widget/block.json', | |
'widgets/src/blocks/widget-group/block.json': 'wp-includes/blocks/widget-group/block.json', | |
...blockFolders.reduce( ( files, blockName ) => { | |
files[ `block-library/src/${ blockName }/block.json` ] = `wp-includes/blocks/${ blockName }/block.json`; | |
return files; | |
} , {} ), | |
}; |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.json
files instead of the.php
files whenWP_DEBUG
and/orSCRIPT_DEBUG
istrue
- 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 👍 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look solid. Great if it merge if core so it call only single file. Left one small change that we should end the PHP tag.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't those paths absolute - which means they'd change from site to site? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.json
files toblock-json.php
block-json.php
files in theregister_block_type_from_metadata()
functionNote: To test this patch, you'll need to run
npm run build
, ornpm run grunt blockJson2PHP
to 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.