Skip to content

Conversation

@oandregal
Copy link
Member

@oandregal oandregal commented Apr 20, 2023

Trac ticket https://core.trac.wordpress.org/ticket/58208
Follow-up to https://core.trac.wordpress.org/ticket/45065

What

This PR removes the wp_enqueue_registered_block_scripts_and_styles callback from the enqueue_block_editor_assets action.

Why

There are two actions to enqueue block assets: enqueue_block_editor_assets and enqueue_block_assets. The former enqueues the assets to the editor and the later enqueues them to the front-and AND the editor.

Given wp_enqueue_registered_block_scripts_and_styles is already bound to the enqueue_block_assets (front-end & editor), it is unnecessary to bind it to enqueue_block_editor_assets (editor) as well.

This was originally introduced at 76525a7 and hasn't been modified since.

How

Removes the action callback from being hooked to enqueue_block_editor_assets.

How to test

  • Install a 3rd party block that contains JS and CSS assets. You can download and use the blockjson-assets test plugin I created.
  • If you use that plugin, verify that a "DOM" message is logged to the console (JS assets are loaded).
  • Insert the "blockjson-assets" block and verify the styles look proper:

image

SVN commit message

Enqueue registered assets once.

This PR removes the `wp_enqueue_registered_block_scripts_and_styles` callback from the `enqueue_block_editor_assets` action.

There are two actions to enqueue block assets: `enqueue_block_editor_assets` and `enqueue_block_assets`. The former enqueues the assets to the editor and the later enqueues them to the front-and and the editor. Given `wp_enqueue_registered_block_scripts_and_styles` is already bound to the `enqueue_block_assets` (front-end & editor), it is unnecessary to bind it to `enqueue_block_editor_assets` (editor) as well.

This was originally introduced at [44157] and hasn't been modified since.

Props ellatrix, costdev.
Fixes #58208.

The wp_enqueue_registered_block_scripts_and_styles callback
is bound to enqueue_block_editor_assets and enqueue_block_assets.

It doesn't need to, as it should be sufficient to bind it only
to enqueue_block_assets. By definition, that hook
fires for both front and editor.
@oandregal
Copy link
Member Author

I ran a performance analysis and haven't detected this PR has any impact. It is still worthwhile to do it right.


For transparency, this is what I've done, so anyone can reproduce if interested.

  • Enable XDEBUG in profile mode using URLs for output. Set the debug variables to production. This is the diff:
diff --git a/.env b/.env
index 63a8169f64..789e9871c6 100644
--- a/.env
+++ b/.env
@@ -18,7 +18,7 @@ LOCAL_DIR=src
 LOCAL_PHP=latest
 
 # Whether or not to enable Xdebug.
-LOCAL_PHP_XDEBUG=false
+LOCAL_PHP_XDEBUG=true
 
 ##
 # The Xdebug features to enable.
@@ -31,7 +31,7 @@ LOCAL_PHP_XDEBUG=false
 #
 # For a full list of accepted values, see https://xdebug.org/docs/all_settings#mode.
 ##
-LOCAL_PHP_XDEBUG_MODE=develop,debug
+LOCAL_PHP_XDEBUG_MODE=profile
 
 # Whether or not to enable Memcached.
 LOCAL_PHP_MEMCACHED=false
@@ -54,10 +54,10 @@ LOCAL_DB_TYPE=mysql
 LOCAL_DB_VERSION=5.7
 
 # The debug settings to add to `wp-config.php`.
-LOCAL_WP_DEBUG=true
-LOCAL_WP_DEBUG_LOG=true
-LOCAL_WP_DEBUG_DISPLAY=true
-LOCAL_SCRIPT_DEBUG=true
+LOCAL_WP_DEBUG=false
+LOCAL_WP_DEBUG_LOG=false
+LOCAL_WP_DEBUG_DISPLAY=false
+LOCAL_SCRIPT_DEBUG=false
 LOCAL_WP_ENVIRONMENT_TYPE=local
 
 # The URL to use when running e2e tests.
diff --git a/docker-compose.yml b/docker-compose.yml
index 739c65f4a8..b781c3fe94 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -39,6 +39,7 @@ services:
     environment:
       - LOCAL_PHP_XDEBUG=${LOCAL_PHP_XDEBUG-false}
       - XDEBUG_MODE=${LOCAL_PHP_XDEBUG_MODE-develop,debug}
+      - XDEBUG_CONFIG=profiler_output_name=cachegrind.out.%R
       - LOCAL_PHP_MEMCACHED=${LOCAL_PHP_MEMCACHED-false}
       - PHP_FPM_UID=${PHP_FPM_UID-1000}
       - PHP_FPM_GID=${PHP_FPM_GID-1000}
  • Install the deps: npm install && npm run build:dev.
  • Start the environment: npm run env:start && npm run env:install.
  • Open a post in the post editor. The URL would be something like this http://localhost:10035/wp-admin/post.php?post=492&action=edit
  • Inspect the generated cachegrind files, find the one relevant to the post editor (look at the URL), and copy it to your machine:
docker exec -it `docker ps -f name=wordpress-develop_php -q` ls -lt --full-time /tmp # List the files and inspect.
docker cp `docker ps -f name=wordpress-develop_php -q`:'/tmp/cachegrind.out._wp-admin_post_php_post=1_action=edit.gz' ~/<path-in-your-machine>
  • Open the file with any application that visualizes valgrind-like files. For example, kcachegrind (qcachegrind for mac).

Repeat this process for trunk and this branch to compare the profiles. I'm sharing here the ones I've extracted in case anyone wants to just visualize them: trunk.gz (at 0f28f4c), pr.gz

Trunk This branch
wp_enqueue_registered_block_scripts_and_styles Called twice and took 0.16ms. Called once and took 0.15ms.

By looking at this function's implementation, I've seen that the number of times wp_enqueue_style is called goes from 18 to 15. Digging a bit into the code, I haven't seen any filter/action that is relevant to have a big impact on performance.

I did notice the total execution time of the request for trunk was 951ms vs 862ms in the branch. While noticeable, I couldn't track the improvement to this change. I attribute the difference to factors other than this change (variability of the profiling, etc.).

@oandregal oandregal requested a review from ellatrix April 21, 2023 08:57
@oandregal oandregal self-assigned this Apr 21, 2023
@spacedmonkey spacedmonkey self-requested a review April 21, 2023 16:21
add_action( 'admin_enqueue_scripts', 'wp_localize_jquery_ui_datepicker', 1000 );
add_action( 'admin_enqueue_scripts', 'wp_common_block_scripts_and_styles' );
add_action( 'enqueue_block_assets', 'wp_enqueue_registered_block_scripts_and_styles' );
add_action( 'enqueue_block_assets', 'enqueue_block_styles_assets', 30 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this priority still make sense? Do you know why it's there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, I misread, I should be looking at the line above.

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍

Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @oandregal! LGTM 👍

@oandregal
Copy link
Member Author

@oandregal
Copy link
Member Author

Follow-up at #4498

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants