Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#59003 closed enhancement (fixed)

Add `wp_get_theme_data_template_parts` function

Reported by: oandregal's profile oandregal Owned by: oandregal's profile oandregal
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.3
Component: General Keywords: has-patch
Focuses: Cc:

Description

Part of https://github.com/WordPress/gutenberg/issues/45171

What

Add a new public function, wp_get_theme_data_template_parts to query the template parts metadata defined by the theme via theme.json. It also substitutes current usage of private APIs.

Why?

We need to offer public APIs for consumers to retrieve the data they need, so they don't resort to using private APIs.

Change History (16)

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


8 months ago
#1

  • Keywords has-patch added

Part of https://github.com/WordPress/gutenberg/issues/45171
Trac ticket: https://core.trac.wordpress.org/ticket/59003

## What

This PR adds a new public function wp_get_theme_template_part_metadata to offer access to metadata stored under templateParts in theme.json.

## Why

There is a no public function to get access to that data, so consumers need to resort to private APIs. This follows the steps of wp_get_global_settings, wp_theme_has_theme_json, or wp_get_theme_directory_pattern_slugs.

## How

It creates a new function that uses the private calls and substitutes all instances of the old one in the codebase.

## Performance

This proves to be a small improvement (less than 5%) in the tests performed.

Tested loading a single post (hello world) with a site configured as production (WP_DEBUG, SCRIPT_DEBUG, WP_DEBUG_LOG, WP_DEBUG_DISPLAY to false).

Taking advantage of the new function, it adds a cache following the steps of similar functions such wp_get_global_settings or wp_get_global_stylesheet. This results in a reduction of calls to WP_Theme_JSON_Resolver::get_theme_data from 15 to 8.

Trunk Branch
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/583546/5a363819-80d3-4887-b23b-9c433fcc0ce3 https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/583546/b5027eb7-11da-4d98-8670-4cb6c9f0eca9

In terms of benchmarking with "production" sites, I've done the same test using different tools for comparison.

  • benchmark-server-timing
npm run research -- benchmark-server-timing -p --url http://localhost:8889/2023/08/08/hello-world/ -n 100
Theme Trunk This PR Improvement
TwentyTwentyThree 165.69ms 168.51ms -2.82ms (1.70% faster)
TwentyTwentyOne 84.66ms 85.49ms -0.82ms (0.98% faster)
  • benchmark-web-vitals
npm run research -- benchmark-web-vitals -p --url http://localhost:8889/2023/08/08/hello-world/ -n 100

TTFB

Theme Trunk This PR Improvement
TwentyTwentyThree 228.3ms 225.95ms -2.35ms (1.02% faster)
TwentyTwentyOne 126.1ms 125.85ms -0.25ms (0.19%faster)

LCP

Theme Trunk This PR Improvement
TwentyTwentyThree 314.9ms 307.15ms -7.75ms (2.46% faster)
TwentyTwentyOne 199.55ms 200.45ms +0.9ms (0.45% slower)

LCP-TTFB

Theme Trunk This PR Improvement
TwentyTwentyThree 92.2ms 92.4ms +0.2ms (0.21% slower)
TwentyTwentyOne 85.95ms 84.45ms -1.5ms (1.74% faster)
  • curl
seq 100 | xargs -Iz curl -o /dev/null -H 'Cache-Control: no-cache' -s -w "%{time_starttransfer}\n" http://localhost:8889 | awk '{sum+=$1} END {print sum/NR}' | pbcopy
Theme Trunk This PR Improvement
TwentyTwentyThree 154.60ms 152.471ms -2.3ms (1.37% faster)
TwentyTwentyOne 65.89ms 65.68ms -0.21ms (0.32% faster)

## How to test

To be determined.

@oandregal commented on PR #4971:


8 months ago
#2

@felixarntz @joemcgill this is ready: created the trac ticket, provided testing instructions and a performance report.

Performance-wise, this has less impact than I originally thought. It is still valuable, though. Specially, because that the main reason for this PR is to introduce a public method to retrieve data from theme.json (consumers have to use private APIs so far).

@oandregal commented on PR #4971:


8 months ago
#3

Side-notes about performance report:

  • It is still costly to prepare one. Would you think https://core.trac.wordpress.org/ticket/58358 could be prioritized? It's a bottleneck to analyze PRs manually. As a result, we do less pre-merge performance analysis. For example, in this PR, where it was not strictly necessary, I've spent a few hours.
  • I'm surprised about the differences depending on the method. Not the numbers themselves, but the ratio for block vs classic themes. Codevitals reports server time as almost equal for classic and blocks while the other methods report classic themes being 2x faster than blocks (curl is more pessimistic and lies in 2.3x with benchmark-* tools being more optimistic around 1.9x).

@flixos90 commented on PR #4971:


8 months ago
#4

@oandregal

I'm surprised about the differences depending on the method. Not the numbers themselves, but the ratio for block vs classic themes. Codevitals reports server time as almost equal for classic and blocks while the other methods report classic themes being 2x faster than blocks (curl is more pessimistic and lies in 2.3x with benchmark-* tools being more optimistic around 1.9x).

I have no idea, but that's definitely something we should research more. I wonder whether there's a notable difference in how the two environments set up the test sites? Otherwise, there's a good chance it is because the automated workflow used for Codevitals uses a headless browser, while benchmark-server-timing does not (similar to curl). I wonder if the headless browser setup adds some overhead that skews the data somehow - or vice versa?

@oandregal commented on PR #4971:


8 months ago
#5

🤔 I just realized the test data in use by CI is different from the “dummy” content that comes by default. I'd need to use that dataset to know whether it affected these tests.

@oandregal commented on PR #4971:


8 months ago
#6

I'm surprised about the differences depending on the method.

I just tried compare-wp-performance by @swissspidy and this is the result of running ./run.sh latest trunk.

wp-total (p50) for classic themes is at 20.88ms while it sits at 31.64ms for block themes, a 1.5x (unlike the ~1x of codevitals and ~2x of curl and benchmark-* tools). We have a slight disagreement on the numbers.

@oandregal commented on PR #4971:


8 months ago
#7

@felixarntz btw, all your feedback has been incorporated. Would you 🟢 this when you have the time, so I can continue follow-up work?

@flixos90 commented on PR #4971:


8 months ago
#8

@oandregal Related to this change, @swissspidy shared some relevant feedback on how creating more and more individual wrapper functions for theme.json data is not exactly a sustainable approach, see Slack message. Wouldn't it be more reasonable to consider implementing caching directly into WP_Theme_JSON_Resolver, or implement a wrapper class that comes with caching?

@oandregal commented on PR #4971:


8 months ago
#9

@oandregal Related to this change, @swissspidy shared some relevant feedback on how creating more and more individual wrapper functions for theme.json data is not exactly a sustainable approach, see Slack message. Wouldn't it be more reasonable to consider implementing caching directly into WP_Theme_JSON_Resolver, or implement a wrapper class that comes with caching?

Happy to re-evaluate caching, though, we need the individual functions: they are the public API. Given the discussion around caching doesn't modify the API, I'm going to commit this to continue follow-up work. Please, let the conversation continue.

So, caching: the resolver already caches data. The issue is that it has to invalidate some of it once per request, resulting in some expensive processes having to run twice. I'm looking into changing how it works to make it run once. Take get_core_data as an example: the next image show all its callees to the right. They are called twice when they should have been called once:

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/583546/831cf13a-8c8b-4ada-83b8-e6af5dc2e667

By introducing the public APIs (and caching "at the edge" when it makes sense, such as for settings, styles, templateParts, etc.) I'm hoping to be able to decouple some of the internal processes that theme.json classes do, removing the unnecessary ones depending on the data being accessed, making sure caches are use effectively, etc.

Hope this helps?

@swissspidy commented on PR #4971:


8 months ago
#10

A class can be a public API too, we don‘t have to add functions for everything. My concern was mostly that I‘m worried we‘ll end up creating X new functions each with its own caching wrapper around that class, leading to poor API design and lots of repetition
Hope that helps!

@oandregal commented on PR #4971:


8 months ago
#11

A class can be a public API too, we don‘t have to add functions for everything.

I know. Though, note that WP_Theme_JSON and WP_Theme_JSON_Resolver classes are indeed private, which means we cannot ask the consumers to use them for accessing data (normal use case).

My concern was mostly that I‘m worried we‘ll end up creating X new functions each with its own caching wrapper around that class, leading to poor API design and lots of repetition

I understand, those are valid concerns I also have. For reference, this is the plan. Note that the plan is not to have a micro-method for everything. The plan is to have five methods. One per top-level keys that can exists in a `theme.json`:

{
  "settings": {},
  "styles": {},
  "templateParts": {},
  "customTemplates": {},
  "patterns": {}
}

And the public API:

  • wp_get_global_settings(). Shipped in 5.9.0.
  • wp_get_global_styles(). Shipped in 5.9.0.
  • wp_get_theme_directory_pattern_slugs. Shipped in 6.3.
  • wp_get_theme_data_template_parts(). Added by this PR.
  • wp_get_theme_data_custom_templates(). TBD.

The rationale for this is that each piece has its own requirements (translated or not translated, how sanitization works, etc.). By having these methods, we can improve how things work by refactoring the private classes/processes as needed without breaking backwards compatibility.

Does this overview alleviate some of your concerns?

#12 @oandregal
8 months ago

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

In 56385:

Themes: add wp_get_theme_data_template_parts function.

Adds a new public function, wp_get_theme_data_template_parts that returns the templateParts defined by the active theme from theme.json. It also substitutes the usage of private APIs by this new API.

Props felixarntz.
Fixes #59003

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


8 months ago
#14

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

This PR adds a missing @ to the return tag in the wp_get_theme_data_template_parts function.

#15 @oandregal
8 months ago

In 56412:

Add missing @ to the return tag in the wp_get_theme_data_template_parts function.

Props audrasjb, johnbillion.
Fixes #59003

Note: See TracTickets for help on using tickets.