Make WordPress Core

Opened 9 months ago

Closed 7 months ago

#59314 closed enhancement (wontfix)

Cache the result of template file lookups

Reported by: joemcgill's profile joemcgill Owned by: thekt12's profile thekt12
Milestone: Priority: normal
Severity: normal Version:
Component: Themes Keywords: 2nd-opinion has-patch
Focuses: performance Cc:

Description

WP makes repeated file_exists operations for the same templates that rarely change. To reduce the overhead of these operations we should consider caching the result of get_query_template and use the cached path directly on subsequent requests.

Related tickets:
https://core.trac.wordpress.org/ticket/12491
https://core.trac.wordpress.org/ticket/40731

Design considerations

The design would need to account for the passed list of template candidates (the second parameter) by checking for a cached value after the {$type}_template_hierarchy hook is fired. If found, this would bypass the subsequent locate_template and locate_block_template calls in this function, otherwise would cache the value of the $template variable before it is passed to the final output filter.

To avoid fatal errors, the cached value should still be run through a file_exists check and fall back to the original locate_template function if false. Other cache invalidation strategies will need to be thought through as part of implementation to avoid these cached values becoming stale.

To keep the number of cache keys to a minimum, we could consider storing these as a single multi-dimensional array rather than distinct values.

Benefits

This would avoid the need to run file operations for the same template on multiple requests, given that these files rarely change unless the whole theme has been replaced. Even so, the performance impact of this change is still inconclusive, as file operations on many systems get natively cached by PHP so the impact may be smaller than anticipated based only on application profiling methods.

Caveats and risks

This benefit would be limited to sites running a persistent object cache since this value is only calculated once per request, unless this is implemented as a transient. While file operations do account for a noticeable percentage of the total time spent on the server during a request, this would only avoid the initial template file lookups, and not any additional template parts that are called during the rendering of a template file.

Since the template hierarchy allows a high level of granularity, even to the level of a specific post ID, this cache would need to account for potentially every URL, which could bloat caches. Additional performance evaluation should be conducted on any implementation before this is committed.

Note: A previous attempt at solving this problem was attempted in #40731 using a static cache instead of an object cache, but was determined to not be a significant enough benefit to warrant merging, so this would need to show clear performance benefits.

Change History (22)

#1 @spacedmonkey
9 months ago

We may also consider using glob to lookup on php / html files, caching that. Then the lookup would be if a path is in an array.

#2 @joemcgill
9 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Moving this to Future Release, since we're so close to the 6.4 beta release.

#3 @johnbillion
9 months ago

  • Keywords 2nd-opinion added

Calls to file_exists() should essentially be free due to the stat cache. I can't imagine we'll be able to increase performance by additionally caching the results in the object cache. Let's get some good before and after figures please.

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


9 months ago

#7 follow-up: @flixos90
8 months ago

@joemcgill Can you share some benchmark updates in regards to @johnbillion's question above? Depending on the expected performance impact of this ticket, I think this would be worth prioritizing for the 6.5 milestone?

#8 in reply to: ↑ 7 @joemcgill
8 months ago

  • Milestone changed from Future Release to 6.5

Replying to flixos90:

@joemcgill Can you share some benchmark updates in regards to @johnbillion's question above? Depending on the expected performance impact of this ticket, I think this would be worth prioritizing for the 6.5 milestone?

Regardless of whether we end up pursuing this change, I think there is value in the initial discovery to better understand the real impact the stat cache has on this part of the application life cycle. I'm going to go ahead and move it into this milestone just to prioritize the research.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


8 months ago

#10 @mukesh27
8 months ago

  • Owner set to mukesh27
  • Status changed from new to assigned

#11 @thekt12
8 months ago

I did a small experiment to see how stat caching improves the performance of file_exists.
I added the following code to the Hello Dolly plugin.

The following code carries out two experiments:
One to measure the effect of stat cache when the file exists and another for when the file doesn't exist.

Each test calls two functions, one to measure the time taken for first file_exists call and another for time taken by 1000 subsequent file_exists checks for the same file.

By dividing the value for 1000 calls by the value of the first call, we will get to know that a caching is effective or not. A very small value indicate that caching is effective. A value larger and closer to 1000 indicates that caching is not effective.


add_action( 'init', 'test_file_not_exists' );
function test_file_not_exists() {
	$filename = plugin_dir_path(__FILE__)."notexists.php";
	$start	= microtime( true );
	if ( file_exists( $filename ) ) {
		echo "The file `$filename` exists <br>";
	} else {
		echo "The file $filename does not exist<br>";
	}
	$end_time = $start - microtime( true );
	$GLOBALS["fcN"] = $end_time; 
	print_r( "Time for First call (A)-> {$end_time} <br>");
}

add_action( 'init', 'test_file_not_exists_2' );
function test_file_not_exists_2() {
	global $fcN;
	$filename = plugin_dir_path(__FILE__)."notexists.php";
	$start	= microtime( true );
	for( $i = 0; $i < 1000; $i++ ) {
		if ( file_exists( $filename ) ) {
			// echo "The file $filename exists";
		} else {
			// echo "The file $filename does not exist";
		}
	}
	$end_time = $start - microtime( true );
	print_r( "Time for 1000 call after first call (B)-> {$end_time} <br>");
	print_r( "Proportion (B/A) - ". ($end_time/$fcN). " <br>");
}

The result obtained was

The file `/var/www/src/wp-content/plugins/hello.php` exists
Time for First call (A)-> -3.8862228393555E-5
Time for 1000 call after first call (B)-> -0.0023980140686035
Proportion (B/A) - 61.705521472393


The file /var/www/src/wp-content/plugins/notexists.php does not exist
Time for First call (A)-> -0.00048208236694336
Time for 1000 call after first call (B)-> -0.33195185661316
Proportion (B/A) - 688.57912957468

The result was nearly the same across multiple rounds.

Findings

Caching internal to file_exists is effective if the file exists. However, it shows minimal benefit if the file we are searching for doesn't exits.


This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


8 months ago

#13 @mukesh27
8 months ago

  • Owner changed from mukesh27 to thekt12

@thekt12 conducted some initial research on this, so I'm going to assign this ticket to him.

#14 @flixos90
8 months ago

Related is the higher level theme file caching discussion in #59719.

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


8 months ago
#15

  • Keywords has-patch added; needs-patch removed

this PR is intended for PoC only at the moment.

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

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


8 months ago

#17 @thekt12
8 months ago

In PoC, we found that it's not so easy to bypass locate_block_template as it updates global $_wp_current_template_content, which is directly consumed by get_the_block_template_html while rendering the template.

If we wish to bypass locate_block_template, we need to persistent cache both
$_wp_current_template_content and $_wp_current_template_id globals (which gets updated by locate_block_template). Caching globals will increase the chance of having an unknown side effect.

I am proceeding with only the file_exists cache now to get the metrics.

This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


8 months ago

#19 @thekt12
8 months ago

I have got some initial performance improvement metrics for file_exists caching only (I didn't cache the template from locate_block_template due to the complexity explained in the previous comment ).

There is a minimal ( just 2 to 3 ms ) but a consistent improvement from a cached file_exists over a non-cached one.

Some initial numbers are documented in the PR for reference -
https://github.com/WordPress/wordpress-develop/pull/5597

#20 @joemcgill
8 months ago

Thanks for updates @thekt12. From your initial research, it was good to see confirmation that PHP's internal caching made these repeated file_exists calls inexpensive. Even though it seemed like there may be some potential improvement for file lookups of files that are known to not exist, the PoC seems to show that the potential seems fairly minimal and likely not worth pursuing further. Do you agree?

It's also interesting to see the side effects that were introduced into locate_block_template() for #58746 makes any improvements to that function more difficult. That may be something worth refactoring if that ends up being a blocker to other improvements we want to make.

This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.


7 months ago

#22 @joemcgill
7 months ago

  • Milestone 6.5 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

Going to go ahead and close this as a wontfix given the lack of impact this change would have. I think a future attempt at something like this or #40731 would need to broaden the improvement beyond reducing file I/O checks.

Note: See TracTickets for help on using tickets.