Make WordPress Core

Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#58674 closed task (blessed) (fixed)

Fix incorrect hook being used as separator between `wp-before-template` and `wp-template` metrics in automated performance tests

Reported by: flixos90's profile flixos90 Owned by: joemcgill's profile joemcgill
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.2
Component: Build/Test Tools Keywords: has-patch
Focuses: performance Cc:

Description

Comparing the WordPress 6.3 Beta Server-Timing benchmarks with the automated performance dashboard, it shows that the wp-before-template and wp-template metrics for classic themes are clearly off in one or the other.

While it is expected that values may differ notably between environments, the relationship between them should always be roughly the same - which is not the case here: wp-before-template has a much higher value in the benchmarks, but a much lower value in the dashboard.

After looking at the implementation, it appears that this difference is caused by the underlying tools using a different hook: The Performance Lab plugin which powers the benchmarks relies on the template_include filter (https://github.com/WordPress/performance/blob/2.4.0/server-timing/defaults.php#L63), while WordPress core's performance tests rely on the template_redirect action (https://github.com/WordPress/wordpress-develop/blob/60b3fe7997d1299b83942c0162020df0483622a3/tests/performance/wp-content/mu-plugins/server-timing.php#L4).

Given that the template_include filter is the very last hook before template output starts, the core implementation should be adjusted to use that filter instead of the template_redirect action, as that action runs before the template loading starts which has a notable cost and is not part of template rendering.

Change History (10)

#1 @flixos90
11 months ago

cc @joemcgill @mukesh27

#2 @joemcgill
11 months ago

  • Keywords reporter-feedback added
  • Owner set to joemcgill
  • Status changed from new to accepted

Thanks @felix, I'm not sure what you're proposing is the right approach here. I would argue that determining which template to load is part of the template loading process, and any server timing related to that process should be included in the reported numbers for the wp-template value. Is there a reason that the template_include filter was chosen for the Performance Lab plugin that should be applied in the automated WP tests?

#3 @flixos90
11 months ago

@joemcgill The purpose of the wp-template metric as initially defined in the Performance Lab plugin is to capture the time that rendering the template takes, i.e. any logic that is triggered directly by the PHP/HTML template loaded. wp-before-template covers the rest, i.e. the entire WordPress initialization logic, which includes determining which template to load.

This separates the "responsibilities" for the two metrics between what is controlled by the specific template file loaded vs by the WordPress bootstrap/initialization process.

#4 @joemcgill
11 months ago

  • Keywords reporter-feedback removed
  • Status changed from accepted to assigned

Thanks for clarifying, @flixos90. I don't see a problem with trying to make these consistent. We'll just want to be mindful that this will impact the historical data that we're collecting. Given how new this whole process is, I think it's fine, as we're expecting to iterate for a while as we're learning how to measure changes effectively.

#5 @joemcgill
11 months ago

  • Type changed from defect (bug) to task (blessed)

Changing this to a task, since it's related to our test tools and not code directly shipped in the release.

#6 @flixos90
11 months ago

Thanks @joemcgill. I think this all goes back to https://github.com/10up/wordpress-develop/pull/7 where that part of the code was implemented. I don't see any discussion around it, so it was probably an oversight from all of us. So I think it's best to change, also agreeing that the change to the metrics is fine at this point, since we're still in "polishing phase" anyway.

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


11 months ago
#7

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

#8 @mukesh27
11 months ago

  • Keywords has-unit-tests removed
  • Version set to 6.2

Thank you, @flixos90, for the ticket.

I have opened a pull request where I made the suggested changes to the action. Could you please review it?

Also, in version 6.2, we introduced automated performance tests, so I have set the appropriate version.

#9 @flixos90
11 months ago

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

In 56162:

Build/Test: Fix incorrect hook being used to separate Server-Timing metrics in performance tests.

The wp-before-template and wp-template metric intend to separate WordPress core's own bootstrap process from any logic that is part of the eventual template loaded. The appropriate hook for that is the template_include filter as that occurs right before the template file is included.

Props mukesh27, joemcgill.
Fixes #58674.

Note: See TracTickets for help on using tickets.