Make WordPress Core

Opened 15 months ago

Closed 10 months ago

Last modified 10 months ago

#57756 closed enhancement (fixed)

Improve performance of get_block_templates function

Reported by: oandregal's profile oandregal Owned by: oandregal's profile oandregal
Milestone: 6.3 Priority: high
Severity: normal Version: 5.8
Component: Themes Keywords: has-patch dev-reviewed
Focuses: performance Cc:

Description

The get_block_templates function is responsible to find block templates that match a given search. The function is provided a query parameter that is used to find the relevant user templates (database) and theme templates (file directory). The query parameter includes data such the slugs of the templates or the areas of the template parts.

There are some optimizations we can do to improve how this code performs.

Change History (35)

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


15 months ago
#1

  • Keywords has-patch added

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

## What

This PR improves the performance of block template resolution.

## Why

In a production-like environment, it provides a two-digit improvement for themes with block templates: +15% for TwentyTwentyThree and +25% for TwentyTwentyTwo.

This change does not affect themes that don't have block templates. The variance reported for classic themes can be ignored (lower than 1ms), and should be attributed to the variance of the doing the tests: template resolution is unaffected for classic themes, given they bail early if they don't support block templates.

https://i0.wp.com/user-images.githubusercontent.com/583546/219651244-4310263d-a116-4e51-b481-c517360b748a.png

## How

The get_block_templates function is responsible to find block templates that match a given search. The function is provided a query parameter that is used to find the relevant user templates (database) and theme templates (file directory). The query parameter includes data such the slugs of the templates or the areas of the template parts.

I've found that the get_block_templates retrieves and processes all block templates provided by the theme, to later ignore the ones that don't match the given slug.

This function can be more performant if it only retrieves/processes the templates that match a given slug, instead of them all.

The following profile shows the impact of this modification:

| Before | After |
| --- | --- |
| https://i0.wp.com/user-images.githubusercontent.com/583546/219652222-2aca88ad-c4b4-4d6f-9b3f-707df626711e.png | https://i0.wp.com/user-images.githubusercontent.com/583546/219652291-3c1f3246-4ac4-49f8-a087-50949f21b48b.png |

  • The time it takes as measured by XDebug profiler: 55ms (before) vs 27ms (after).
  • The number of times the _build_block_template_result_from_file is called: 22 (before) vs 5 (after).

## How to test

  • Start the bundled environment.
  • Set .env to production by applying the following patch:

{{{diff
diff --git a/.env b/.env
index 63a8169f64..9b1d52ac50 100644
--- a/.env
+++ b/.env
@@ -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.

}}}

  • Run 100 test of the homepage: seq 100 | xargs -Iz curl -o /dev/null -H 'Cache-Control: no-cache' -s -w "%{time_starttransfer}\n" http://localhost:8889 | xclip -selection clipboard.
  • Copy the results to a spreadsheet like this one to measure the changes.

## TODO

  • $fits_slug_query can be probably removed.
  • Review whether the area can be filtered earlier as well.
  • Decide whether this should be prepared for Gutenberg first. I submitted it for core because it modifies functions that are no longer there:
    • _get_block_templates_files: no longer present.
    • get_block_templates: the corresponding gutenberg_get_block_templates is still present, but it'll be removed once WordPress 6.2 is published. It won't be present in Gutenberg when this PR lands (WordPress 6.3 cycle).

#2 @oandregal
15 months ago

I don't find how to milestone this ticket (probably I don't have the right permissions?). This should be ticketed for WordPress 6.3.

@oandregal commented on PR #4097:


15 months ago
#3

ok, this is now ready to review :) cc @youknowriad @ockham @creativecoder @carlomanf as people who has worked in this area, according to https://github.com/WordPress/wordpress-develop/pull/1267

@oandregal commented on PR #4097:


15 months ago
#4

The perf job reports the following (copy/pasted here in a single table, otherwise it's difficult to compare the results):

  • Home classic theme

| Metric | This PR | Base | Variance |
| --- | --- | --- | --- |
| wpBeforeTemplate | 24.32 | 25.33 | +3.98% |
| wpTemplate | 50.67 | 56.97 | +11.05% |
| wpTotal | 75.28 | 80.32 | +6.25% |

  • Home block theme

| Metric | This PR | Base | Variance |
| --- | --- | --- | --- |
| wpBeforeTemplate | 28.33 | 31.80 | +10.91% |
| wpTemplate | 53.37 | 71.39 | +25.24% |
| wpTotal | 81.34 | 102.74 | +20.82% |

@youknowriad commented on PR #4097:


14 months ago
#5

Can't talk about the performance impact but this looks great to me. Thanks for the updates @oandregal

#6 @mukesh27
14 months ago

  • Keywords changes-requested added
  • Milestone changed from Awaiting Review to 6.3

Thanks @oandregal for ticket and PR.

I left some review feedback on PR. Moving this to 6.3 for consideration.

#7 @mukesh27
14 months ago

  • Keywords changes-requested removed

Thanks @oandregal for the changes.

@oandregal commented on PR #4097:


13 months ago
#8

Thanks @oandregal for the changes. Can you share updated performance number?

I've rebased this upon trunk in preparation for commiting, and updated the issue description with the numbers reported by the perf CI job.

@flixos90 commented on PR #4097:


13 months ago
#9

@oandregal How did you get the numbers from the perf CI job? I'm asking since they only really run for trunk commits as far as I know. Did you trigger it yourself and look at the workflow run logs? The fact that classic themes see a notable improvement strikes me as potentially incorrect, since the change here should only affect block themes like you're saying.

#10 @flixos90
13 months ago

  • Keywords commit added

The PR https://github.com/WordPress/wordpress-develop/pull/4097 brings a massive performance win, with overall load time for block themes being ~15% faster!

This is the full benchmark data: https://docs.google.com/spreadsheets/d/1saGwDWET6ku-oP2BFfZzhTfwMuZZuzRTKxgyUM-i4-k/edit

The WordPress initialization process before rendering the template wp-before-template is where all these improvements occur, that metric is relatively speaking even more pronounced with ~20% improvement.

Kudos @oandregal, it is safe to say already at this point that this will be one of the most important performance wins for WordPress 6.3, if not the most important. But let's keep pushing :)

@oandregal commented on PR #4097:


13 months ago
#11

@oandregal How did you get the numbers from the perf CI job? I'm asking since they only really run for trunk commits as far as I know. Did you trigger it yourself and look at the workflow run logs?

:thinking:

Oh. I looked at the performance CI job that ran for this PR. My understanding was that the numbers under "Print performance test results" was this PR and the numbers under "Print base line test results" was the baseline to compare to. Is that how should I look at them @felixarntz ?

The first time I ran my own numbers they were on the same range that the CI job, so I didn't bother doing my own again. Happy to if I'm reading this wrong.

The fact that classic themes see a notable improvement strikes me as potentially incorrect, since the change here should only affect block themes like you're saying.

Yeah, I don't understand this either. I presumed it was related to the variability of the tests.

@flixos90 commented on PR #4097:


13 months ago
#12

@oandregal

Oh. I looked at the performance CI job that ran for this PR. My understanding was that the numbers under "Print performance test results" was this PR and the numbers under "Print base line test results" was the baseline to compare to. Is that how should I look at them @felixarntz ?

Ah that may be a misunderstanding and slightly confusing. The "base line test results" are not the current stats for trunk, but rather a re-run of getting metrics for trunk in the same baseline state that was originally measured (which I believe we have set to the last commit hash of WP 6.1.1). So this isn't the state of latest trunk.

Looking at the printed "performance test results" is a great idea though. What you need to compare that too though is the similar data for the last commit in trunk that is in your branch here.

All of that is of course not how this should work, but to be fair the performance CI workflow in its current stage was not yet meant to be used to assess PRs. Reporting diffs between the last commit in the branch and the PR automatically in a PR comment is high on the todo-list though :)

#13 @oandregal
13 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55687:

Themes: improve performance of get_block_templates().

get_block_templates() is responsible for finding block templates that match a given search. The function receives a query parameter with the relevant metadata (slugs of the templates, areas of the template parts, etc) to find the user templates (database) and theme templates (file directory).

This function can be made more performant by changing how it works. Before this change, it processed all the block templates and discarded the ones that didn't match the query after it occurred. This commit makes it so it discards the templates that don't match the query before processing them. As a result, it only has to process the subset of templates that will be used, instead of all of them.

This change impacts any theme with block templates. TwentyTwentyThree reports a 15% improvement in Time To First Byte.

Props spacedmonkey, jorgefilipecosta, youknowriad, flixos90, mukesh27.
Fixes #57756.

#14 @oandregal
13 months ago

  • Component changed from General to Themes

#16 @desrosj
13 months ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Worked with @oandregal to get this committed. Great work all!

I did have one follow up question. When I was reading through it, I noticed the inline comment preceding a few template related function calls. It's something like:

/*
 * Structure of a wp_template item:
 *
 * - slug
 * - path
 * - theme
 * - type
 * - title (optional)
 * - postTypes (optional)
 */

My brain rejected this and I think was looking for something similar to @param array $template {} syntax. I know that's not correct and this is just an inline comment block, but I'm wondering if there's a better way to document this without repetition.

Instead, could we possibly just expand the $template_item documentation for _add_block_template_info() and _add_block_template_part_area_info() to fully document what a template contains and let that speak to the structure of a wp_template item?

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


13 months ago
#17

It was brought up at https://core.trac.wordpress.org/ticket/57756#comment:16 that some inline comments introduced at https://github.com/WordPress/wordpress-develop/pull/4097 could be graduated to a PHP DocBlock for better maintenance: use single place for documenting them as well as enable the existing automated docs to pick them up.

This PR removes the inline comments and makes them part of the _get_block_template_file().

---

SVN commit message:

{{{txt
Docs: describe return type of _get_block_template_file().

Props: desrosj.
Fixes #57756.
}}}

@oandregal commented on PR #4388:


13 months ago
#18

An alternative I considered was to make them part of the functions _add_block_template_part_area_info() and _add_block_template_info for wp_template_part and wp_template, respectively.

The role of these functions is to augment the metadata of the template (title + area, or title + postTypes) provided as a parameter. These functions only work with a subset of the template properties (slug as input). It doesn't feel right to have them document what the respective template structure is for a lack of a better place.

@oandregal commented on PR #4388:


13 months ago
#20

@mukeshpanchal27 @desrosj is there any other feedback you want addressed?

@oandregal commented on PR #4388:


13 months ago
#21

@mukeshpanchal27 @costdev @desrosj anyone available to give me a green check on this one, so I can commit?

#22 @oandregal
13 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 55744:

Docs: describe return type of _get_block_template_file().

Props: desrosj, mukesh27, costdev, johnbillion.
Fixes #57756.

#24 @flixos90
11 months ago

  • Priority changed from normal to high

Marking this as "high" despite already merged just as it is worth highlighting due to its notable performance impact on WordPress execution time.

@oandregal commented on PR #4097:


10 months ago
#25

This has created an issue with child themes, see instructions at https://github.com/WordPress/gutenberg/issues/53138 I'm looking into this.

#26 @oandregal
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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


10 months ago
#27

Trac ticket https://core.trac.wordpress.org/ticket/57756
Follow-up to https://github.com/WordPress/wordpress-develop/pull/4097
Fixes https://github.com/WordPress/gutenberg/issues/53138

## What

While improving the performance of the block template retrieval at https://github.com/WordPress/wordpress-develop/pull/4097/ the condition to skip templates from the parent theme that were already defined by the child theme was unintentionally removed. This PR brings it back.

## Why

It's a regression from current behavior.

## How to test

See instructions at https://github.com/WordPress/gutenberg/issues/53138

@ramonopoly commented on PR #4940:


10 months ago
#29

Thanks for the quick patch 🙇🏻

#30 @isabel_brison
10 months ago

In 56329:

Editor: fix duplication in templates list.

Excludes parent template when a child template is defined during template retrieval.

Props oandregal, mukesh27, flixos90, bgardner.
See #57756.

@isabel_brison commented on PR #4940:


10 months ago
#31

Committed to trunk in r56329.

#32 @isabel_brison
10 months ago

  • Keywords dev-reviewed added

#33 @peterwilsoncc
10 months ago

[56329] approved for merging to the 6.3 branch.

#34 @isabel_brison
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 56330:

Editor: fix duplication in templates list.

Excludes parent template when a child template is defined during template retrieval.

Props oandregal, mukesh27, flixos90, bgardner, peterwilsoncc.
Reviewed by peterwilsoncc.
Merges [56329] to the 6.3 branch.
Fixes #57756.

@isabel_brison commented on PR #4940:


10 months ago
#35

Committed to 6.3 in r56330.

Note: See TracTickets for help on using tickets.