Opened 14 months ago
Closed 12 months ago
#59314 closed enhancement (wontfix)
Cache the result of template file lookups
Reported by: | joemcgill | Owned by: | 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)
#2
@
14 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
@
14 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.
14 months ago
This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.
14 months ago
This ticket was mentioned in Slack in #core-performance by thekt12. View the logs.
14 months ago
#7
follow-up:
↓ 8
@
13 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
@
13 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.
13 months ago
#11
@
13 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.
13 months ago
#13
@
13 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.
This ticket was mentioned in PR #5597 on WordPress/wordpress-develop by @thekt12.
12 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.
12 months ago
#17
@
12 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.
12 months ago
#19
@
12 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
@
12 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.
12 months ago
#22
@
12 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.
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.