Make WordPress Core

Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#58385 closed enhancement (fixed)

Skip file_exists check for core blocks in register_block_type_from_metadata

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: high
Severity: normal Version: 5.5
Component: Editor Keywords: has-patch
Focuses: performance Cc:

Description

In the register_block_type_from_metadata function, there is a call to file_exists to see if the metadata file existing before registering. However, it is extremely unlikely the core blocks would not exist. As file lookups are expensive, saving 91 file_exists would improve site performance.

Change History (10)

This ticket was mentioned in PR #4492 on WordPress/wordpress-develop by @spacedmonkey.


12 months ago
#1

  • Keywords has-patch added

#2 @spacedmonkey
12 months ago

@gziolo Do you mind taking a look at my PR. It is very simple.

@presstoke commented on PR #4492:


12 months ago
#3

This makes sense to me. In the same vein of avoiding unneeded file_exists it looks like there’s one nearby in register_block_type:

https://github.com/WordPress/wordpress-develop/blob/f5e657c281c28c1cbda5a71201f3f014fbe4e599/src/wp-includes/blocks.php#L539-L541

The check is extraneous there since it's going to be done in register_block_type_from_metadata. Does that seem right? Core blocks aren’t using that code path but plenty of third parties may be.

@spacedmonkey commented on PR #4492:


12 months ago
#4

This makes sense to me. In the same vein of avoiding unneeded file_exists it looks like there’s one nearby in register_block_type:

https://github.com/WordPress/wordpress-develop/blob/f5e657c281c28c1cbda5a71201f3f014fbe4e599/src/wp-includes/blocks.php#L539-L541

The check is extraneous there since it's going to be done in register_block_type_from_metadata. Does that seem right? Core blocks aren’t using that code path but plenty of third parties may be.

@stokesman Already done in https://github.com/WordPress/wordpress-develop/commit/537eb2145a356b471f474927ab8458ddef6802c1

@presstoke commented on PR #4492:


12 months ago
#5

True that it’s now side-stepped by core blocks but any calls to register_block_type with a string for the first parameter are checking file_exists and forwarding directly to register_block_type_from_metadata where it again checks file_exists. It seems like the first one can be dropped, no?

Do excuse me if it's not really related enough to be bringing up on this PR but it did seem so.

#6 @spacedmonkey
12 months ago

  • Owner set to spacedmonkey
  • Resolution set to fixed
  • Status changed from new to closed

In 55910:

Editor: Skip file_exist check for core blocks.

In register_block_type_from_metadata function, skip calling file_exists on core blocks. Core blocks are part of the codebase and will never not exist. Not calling this function is better for performance, as the file lookup can be expensive.

Props spacedmonkey, joemcgill.
Fixes #58385.

@spacedmonkey commented on PR #4492:


12 months ago
#7

Committed.

#8 @spacedmonkey
12 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#9 @spacedmonkey
12 months ago

  • Milestone changed from Future Release to 6.3
  • Resolution set to fixed
  • Status changed from reopened to closed
  • Version set to 5.5

#10 @flixos90
12 months ago

After conducting some benchmarks here, I can say that the performance difference is negligible. Doing various benchmarks with 100 runs each for [55910] compared to the previous commit, sometimes this commit is slightly faster, other times the previous commit is slightly faster - which means it is due to variance, so the impact is too small to be notable. However, even with that it's clear to tell from the change that it is beneficial.

Let's keep in mind for the future that file_exists() checks are particularly expensive for when the files don't exist, based on previous findings. Since for WP core files we know that they always exist, that's probably why removing them didn't bring a notable performance win. Still probably good to have done it :)

Note: See TracTickets for help on using tickets.