Make WordPress Core

Opened 16 months ago

Closed 8 months ago

Last modified 5 weeks ago

#55005 closed enhancement (fixed)

Improve PHP performance for block.json files

Reported by: aristath's profile aristath Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch needs-testing needs-unit-tests has-dev-note
Focuses: performance Cc:

Description

Ticket initially created in https://github.com/WordPress/gutenberg/issues/37611, but this will probably need to be done in WP-Core instead, so cross-posting here.

The block.json convention is extremely convenient, however it does add a few extra steps when core blocks get instantiated:

  • We go through folders and look for block.json files
  • When we locate a file, we read its contents
  • json_decode the file contents
  • Register the block using the decoded data

All these things happen on each page load. We could improve the performance and save some processing for all sites by doing one of 2 things:

  • Cache the data for blocks
  • Add an extra task in our compilers, saving the decoded JSON data as a PHP array

Caching the data can be a little complicated because we'd still need to check the JSON file on each page-load to make sure there have been no changes to the file (maybe using a filemtime call - which would still be more effective than reading the file contents & decoding it). Caching would also require a DB call - which would still be better than the current processes that run, but not as optimal as the alternative below.

Adding an extra task and compile the decoded JSON data as a PHP array seems like a better choice. It would be a lot easier to parse a native PHP array on each page-load, and we can also take care of development tasks etc.
We already use the json2php package in Gutenberg & Core, so adding a new task for this should not be a difficult task.

Change History (32)

This ticket was mentioned in PR #2252 on WordPress/wordpress-develop by aristath.


16 months ago
#1

  • Keywords has-patch added

This PR improves the PHP performance of blocks:

  • Adds a new Grunt task to convert block.json files to block-json.php
  • Uses the new block-json.php files in the register_block_type_from_metadata() function

Note: To test this patch, you'll need to run npm run build, or npm run grunt blockJson2PHP to generate the PHP files from JSON.

Trac ticket: https://core.trac.wordpress.org/ticket/55005

gziolo commented on PR #2252:


16 months ago
#2

How more performant does this become when we replace JSON files with PHP files? It's maybe 50-60 files to process.

aristath commented on PR #2252:


16 months ago
#3

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 @wordpress/env but so far haven't found a solution. Any ideas?

aristath commented on PR #2252:


16 months ago
#4

Not exactly a concrete result, but it does confirm that there is indeed an improvement.
I used this code:

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/button/block.json' );
		register_block_type_from_metadata( '/var/www/src/wp-includes/blocks/cover/' );
		register_block_type_from_metadata( '/var/www/src/wp-includes/blocks/cover/block.json' );
	}
	$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)
After the patch, it dropped to ~4.23s (max: 5)

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:
When I run npm run grunt blockJson2PHP, it generates 71 files so I'm assuming that's how many times the function will run on a normal site. My loop runs the function 4000 times (4 calls x 1000 loops), so 56.34 times more than WP Core would. The difference of my pre/post runs was 2.15s (6.38s - 4.23s), so dividing that by 56.34 I get 0.038s, or 38ms.
That sounds like a lot so my results are probably false, but even if it's just a tenth of that, it's still 4ms per-page-load on 40% of the web 😅

gziolo commented on PR #2252:


16 months ago
#5

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.

aristath commented on PR #2252:


16 months ago
#6

Hmmm it would make sense... and we'd only have to load a single file. Great idea 👍

aristath commented on PR #2252:


16 months ago
#7

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.


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 👍

#8 @hellofromTonya
12 months ago

  • Component changed from General to Editor
  • Keywords needs-testing has-unit-tests added
  • Milestone changed from Awaiting Review to 6.1
  • Owner set to hellofromTonya
  • Status changed from new to assigned

Moving this performance enhancement into 6.1 and self-assigning. It'll need a lot of testing in different environments, unit tests (i.e. a review to determine if there's coverage for the changes), and performance benchmarking.

I agree that in-memory caching should yield performance gains in comparison to multiple hits to the filesystem. That hypothesis though will need to validated.

Thought and analysis is needed to determine:

  • if there is a threshold for "in-memory" consumption. What do I mean? At what point does the size of the memory usage for in-memory metadata become less performant than fetching needed block .json files?
  • is there a potential for running out of memory on hosting plans that might be throttled?

johnbillion commented on PR #2252:


12 months ago
#9

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.

#10 @ironprogrammer
11 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed

Minor keyword admin for reports.

#11 @petitphp
10 months ago

As per discussions in core-performance bug scrub, should we also explore the same king of performance improvement for the loading of theme.json file ?

#12 @adamsilverstein
9 months ago

Hey @hellofromTonya & @aristath - this ticket came up in the performance bug scrub today and I wanted to check on the prospects of landing this in 6.1, what do you think?

Reviewing the ticket and PR it looks like this left off needing more solid testing around the performance benefits. In addition, @johnbillion asked about cache invalidation, is that covered in this ticket?

#13 @aristath
9 months ago

Hey @hellofromTonya & @aristath - this ticket came up in the performance bug scrub today and I wanted to check on the prospects of landing this in 6.1, what do you think?

Landing this in 6.1 would be the desired outcome.
This patch doesn't break backwards-compatibility in any way, and simply improves PHP performance for Core blocks by preventing 70+ file-reads and json-decoding calls.

Reviewing the ticket and PR it looks like this left off needing more solid testing around the performance benefits.

There's no argument that this does indeed improve performance. In my tests the performance gain was ~33%, but that's anecdotal and I'm guessing it would range between 10-40% depending on the environment. Still... far from negligible.

In addition, @johnbillion asked about cache invalidation, is that covered in this ticket?

There is no database cache, so nothing to invalidate. The "cache" here is a compiled PHP file, generated from the block.json files. If a block.json file changes, then we'd have to run npm run build to update the JS files anyway, and when doing that the PHP file gets recompiled at the same time so I don't think that cache-invalidation is of any concern here 👍

#14 @adamsilverstein
8 months ago

Thanks for clarifying. Note, tests are failing on the PR.

SergeyBiryukov commented on PR #2252:


8 months ago
#15

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:

  1. The "Ensure version-controlled files are not modified or deleted" job in E2E, JavaScript, and NPM tests reports some differences in the wp-includes/blocks/blocks-json.php file. When inspecting the differences, it looks like they have to do with this line being present in a few blocks before the change:
    'fontSize' => array('type' => 'string', 'default' => 'small')
    
    • core/comment-date
    • core/comment-edit-link
    • core/comment-reply-link
    • core/comment-template

and not being present after the change.

  1. Some PHPUnit test failures (see below). Reading through the PR, it's not quite clear to me what exactly causes these. Hopefully this should provide some details to help with further debugging for someone more familiar with the code 🙂
    There were 3 errors:
    
    1) Tests_Blocks_Render::test_do_block_output with data set #12 ('core__cover.html', 'core__cover.server.html')
    Undefined index: useFeaturedImage
    
    /var/www/src/wp-includes/blocks/cover.php:17
    /var/www/src/wp-includes/class-wp-block.php:256
    /var/www/src/wp-includes/blocks.php:1027
    /var/www/src/wp-includes/blocks.php:1065
    /var/www/tests/phpunit/tests/blocks/render.php:[https://github.com/WordPress/wordpress-develop/actions/runs/3083280374/jobs/4984017435#step:17:226 225]
    phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
    /var/www/vendor/bin/phpunit:118
    
    2) Tests_Blocks_Render::test_do_block_output with data set #33 ('core__latest-comments.html', 'core__latest-comments.server.html')
    Undefined index: displayDate
    
    /var/www/src/wp-includes/blocks/latest-comments.php:124
    /var/www/src/wp-includes/class-wp-block.php:256
    /var/www/src/wp-includes/blocks.php:1027
    /var/www/src/wp-includes/blocks.php:1065
    /var/www/tests/phpunit/tests/blocks/render.php:225
    phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
    /var/www/vendor/bin/phpunit:118
    
    3) Tests_Blocks_Render::test_render_latest_comments_on_password_protected_post
    Undefined index: displayAvatar
    
    /var/www/src/wp-includes/blocks/latest-comments.php:65
    /var/www/src/wp-includes/class-wp-block.php:256
    /var/www/src/wp-includes/blocks.php:1027
    /var/www/src/wp-includes/blocks.php:1065
    /var/www/tests/phpunit/tests/blocks/render.php:329
    phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
    /var/www/vendor/bin/phpunit:118
    
    --
    
    There were 5 failures:
    
    1) Tests_Blocks_Render::test_do_block_output with data set #8 ('core__column.html', 'core__column.server.html')
    File '/var/www/tests/phpunit/includes/../data/blocks/fixtures/core__column.html' does not match expected value
    Failed asserting that two strings are identical.
    --- Expected
    +++ Actual
    @@ @@
     
    -<div class="wp-container-1 wp-block-column">
    +<div class="wp-block-column">
     	
     	<p>Column One, Paragraph One</p>
     	
     	
     	<p>Column One, Paragraph Two</p>
     	
     </div>
    
    /var/www/tests/phpunit/tests/blocks/render.php:[https://github.com/WordPress/wordpress-develop/actions/runs/3083280374/jobs/4984017435#step:17:241 240]
    phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
    /var/www/vendor/bin/phpunit:118
    
    2) Tests_Blocks_Render::test_do_block_output with data set #9 ('core__columns.html', 'core__columns.server.html')
    File '/var/www/tests/phpunit/includes/../data/blocks/fixtures/core__columns.html' does not match expected value
    Failed asserting that two strings are identical.
    --- Expected
    +++ Actual
    @@ @@
     
     <div class="wp-container-1 wp-block-columns has-3-columns">
     	
    -	<div class="wp-container-1 wp-block-column">
    +	<div class="wp-block-column">
    @@ @@
     	
    -	<div class="wp-container-1 wp-block-column">
    +	<div class="wp-block-column">
     		
     		<p>Column Two, Paragraph One</p>
     		
     		
     		<p>Column Three, Paragraph One</p>
     		
     	</div>
     	
     </div>
    
    /var/www/tests/phpunit/tests/blocks/render.php:240
    phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
    /var/www/vendor/bin/phpunit:118
    
    3) Tests_Blocks_RenderReusableCommentTemplate::test_rendering_comment_template
    Failed asserting that two strings are identical.
    --- Expected
    +++ Actual
    @@ @@
    -<ol class="wp-block-comment-template"><li id="comment-40" class="comment even thread-even depth-1"><div class="wp-block-comment-author-name"><a rel="external nofollow ugc" href="http://example.com/author-url/" target="_self" >Test</a></div><div class="wp-block-comment-content"><p>Hello world</p></div></li></ol>
    +<ol class="wp-block-comment-template"><li id="comment-40" class="comment even thread-even depth-1"><div class="wp-block-comment-author-name">Test</div><div class="wp-block-comment-content"><p>Hello world</p></div></li></ol>
    
    /var/www/tests/phpunit/tests/blocks/renderCommentTemplate.php:[https://github.com/WordPress/wordpress-develop/actions/runs/3083280374/jobs/4984017435#step:17:286 285]
    phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
    /var/www/vendor/bin/phpunit:118
    
    4) Tests_Blocks_RenderReusableCommentTemplate::test_rendering_comment_template_nested
    Failed asserting that two strings are identical.
    --- Expected
    +++ Actual
    @@ @@
    -<ol class="wp-block-comment-template"><li id="comment-41" class="comment even thread-even depth-1"><div class="wp-block-comment-author-name"><a rel="external nofollow ugc" href="http://example.com/author-url/" target="_self" >Test</a></div><div class="wp-block-comment-content"><p>Hello world</p></div><ol><li id="comment-42" class="comment odd alt depth-2"><div class="wp-block-comment-author-name"><a rel="external nofollow ugc" href="http://example.com/author-url/" target="_self" >Test</a></div><div class="wp-block-comment-content"><p>Hello world</p></div><ol><li id="comment-44" class="comment even depth-3"><div class="wp-block-comment-author-name"><a rel="external nofollow ugc" href="http://example.com/author-url/" target="_self" >Test</a></div><div class="wp-block-comment-content"><p>Hello world</p></div></li></ol></li><li id="comment-43" class="comment odd alt depth-2"><div class="wp-block-comment-author-name"><a rel="external nofollow ugc" href="http://example.com/author-url/" target="_self" >Test</a></div><div class="wp-block-comment-content"><p>Hello world</p></div></li></ol></li></ol>
    +<ol class="wp-block-comment-template"><li id="comment-41" class="comment even thread-even depth-1"><div class="wp-block-comment-author-name">Test</div><div class="wp-block-comment-content"><p>Hello world</p></div><ol><li id="comment-42" class="comment odd alt depth-2"><div class="wp-block-comment-author-name">Test</div><div class="wp-block-comment-content"><p>Hello world</p></div><ol><li id="comment-44" class="comment even depth-3"><div class="wp-block-comment-author-name">Test</div><div class="wp-block-comment-content"><p>Hello world</p></div></li></ol></li><li id="comment-43" class="comment odd alt depth-2"><div class="wp-block-comment-author-name">Test</div><div class="wp-block-comment-content"><p>Hello world</p></div></li></ol></li></ol>
    
    /var/www/tests/phpunit/tests/blocks/renderCommentTemplate.php:391
    phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
    /var/www/vendor/bin/phpunit:118
    
    5) Tests_Blocks_RenderReusableCommentTemplate::test_rendering_comment_template_unmoderated_preview
    Should include unapproved comments when filter applied
    Failed asserting that two strings are identical.
    --- Expected
    +++ Actual
    @@ @@
    -<ol class="wp-block-comment-template"><li id="comment-47" class="comment even thread-even depth-1"><div class="wp-block-comment-author-name"><a rel="external nofollow ugc" href="http://example.com/author-url/" target="_self" >Test</a></div><div class="wp-block-comment-content"><p>Hello world</p></div></li><li id="comment-48" class="comment odd alt thread-odd thread-alt depth-1"><div class="wp-block-comment-author-name">Visitor</div><div class="wp-block-comment-content"><p><em class="comment-awaiting-moderation">Your comment is awaiting moderation.</em></p>Hi there! My comment needs moderation.</div></li></ol>
    +<ol class="wp-block-comment-template"><li id="comment-47" class="comment even thread-even depth-1"><div class="wp-block-comment-author-name">Test</div><div class="wp-block-comment-content"><p>Hello world</p></div></li><li id="comment-48" class="comment odd alt thread-odd thread-alt depth-1"><div class="wp-block-comment-author-name">Visitor</div><div class="wp-block-comment-content"><p><em class="comment-awaiting-moderation">Your comment is awaiting moderation.</em></p>Hi there! My comment needs moderation.</div></li></ol>
    
    /var/www/tests/phpunit/tests/blocks/renderCommentTemplate.php:510
    phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:51
    /var/www/vendor/bin/phpunit:118
    

This ticket was mentioned in Slack in #core by chaion07. View the logs.


8 months ago

aristath commented on PR #2252:


8 months ago
#17

I rebased too and also ran npm run build again to regenerate the block-json.php file.
The test failures I see don't make any sense and I don't see how they could be related to this PR...
@gziolo do you have any idea what the issue might be here? 🤔

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 months ago

#19 @JeffPaul
8 months ago

  • Milestone changed from 6.1 to Future Release

As discussed ahead of today's scheduled 6.1 Beta 1 release party (see the Slack link above), it was deemed that this ticket was not ready and should be punted so I'm moving to the Future Release milestone from where it can be moved to a numbered release by those active here.

#20 @aristath
8 months ago

The issues mentioned above were caused by an outdated package, and it has been fixed in the PR already. Tests now pass properly.

Since this is not strictly an enhancement but more of a performance bugfix, can we reconsider adding it to v6.1? It saves a lot of calls to get & decode JSON files, and in my tests with Xdebug profiling it saves ~100-200ms on each pageload.

Last edited 8 months ago by aristath (previous) (diff)

gziolo commented on PR #2252:


8 months ago
#21

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 block-json.php file.

mukeshpanchal27 commented on PR #2252:


8 months ago
#22

@aristath I think it's good to set 6.1 milestone as it's ready for review and merge.

#23 @SergeyBiryukov
8 months ago

  • Milestone changed from Future Release to 6.1
  • Resolution set to fixed
  • Status changed from assigned to closed

In 54276:

Editor: Improve block loading PHP performance.

This commit improves PHP performance for core blocks by reading a single PHP file with block metadata, instead of reading a JSON file per-block and then decoding from JSON to PHP.

Includes:

  • Adding a new Grunt task to convert block.json files to block-json.php.
  • Using the new block-json.php file in the register_block_type_from_metadata() function.

Follow-up to [48141].

Props aristath, gziolo, johnbillion, presstoke, mukesh27, hellofromTonya, petitphp, adamsilverstein, costdev, desrosj, SergeyBiryukov.
Fixes #55005.

SergeyBiryukov commented on PR #2252:


8 months ago
#24

Thanks for the PR! Merged in r54276.

gziolo commented on PR #2252:


8 months ago
#25

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!

#26 @milana_cap
8 months ago

  • Keywords needs-dev-note added

#27 @martin.krcho
8 months ago

Someone on the support forum confirmed that the performance fix worked for them :)

https://wordpress.org/support/topic/wp5-8-adds-too-many-fopen-syscalls-and-cause-10-performance-drop/

They previously reported too many fopen syscalls and tested again with WordPress 6.1 Beta 1 yesterday => performance issues are gone.

#28 @aristath
8 months ago

Thank you for the followup @martinkrcho!

justlevine commented on PR #2252:


8 months ago
#29

I'm not seeing any hooks allowing devs to load their own block-json.php. Is that because

  • there is an existing hook we can tap into (if so, what is it),
  • intentional design (if so, is there an alternative method for custom block registration to reap similar performance improvements), or
  • an oversight (if so is there already a ticket, and/or will it be included in 6.1)?

@aristath commented on PR #2252:


8 months ago
#31

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 👍

CreativeDive commented on PR #2252:


5 weeks ago
#32

hey @aristath I saw that you added improvements to avoid loading the block.json file for each block type. But I noticed that this is not available for custom blocks. Personally I have more than 50 custom blocks and it would be very helpful to make the same improvements for custom blocks.

All we need is just a filter and everyone can create their own logic to load block data more efficiently.

Changing the existing part ...

{{{php
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 ...

{{{php
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', $file_or_folder );

}

}}}

... and that's all. Is this something that can be extended and should I open a new issue for it?

Note: See TracTickets for help on using tickets.