Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 20 months 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.


3 years 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:


3 years 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:


3 years 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:


3 years 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:


3 years 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:


3 years ago
#6

Hmmm it would make sense... and we'd only have to load a single file. Great idea ๐Ÿ‘

โ€‹aristath commented on โ€‹PR #2252:


3 years 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
2 years 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:


2 years 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
2 years ago

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

Minor keyword admin for reports.

#11 @petitphp
2 years 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
2 years 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
2 years 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
2 years ago

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

โ€‹SergeyBiryukov commented on โ€‹PR #2252:


2 years 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.


2 years ago

โ€‹aristath commented on โ€‹PR #2252:


2 years 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.


2 years ago

#19 @JeffPaul
2 years 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
2 years 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 2 years ago by aristath (previous) (diff)

โ€‹gziolo commented on โ€‹PR #2252:


2 years 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:


2 years ago
#22

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

#23 @SergeyBiryukov
2 years 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:


2 years ago
#24

Thanks for the PR! Merged in r54276.

โ€‹gziolo commented on โ€‹PR #2252:


2 years 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
2 years ago

  • Keywords needs-dev-note added

#27 @martin.krcho
2 years 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
2 years ago

Thank you for the followup @martinkrcho!

โ€‹justlevine commented on โ€‹PR #2252:


2 years 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:


2 years 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:


20 months 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.