![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
Opened 6 months ago
Closed 2 months ago
#64538 closed defect (bug) (fixed)
memoize wp_normalize_path
| Reported by: | josephscott | Owned by: | dmsnell |
|---|---|---|---|
| Priority: | normal | Milestone: | 7.0 |
| Component: | General | Version: | 3.9 |
| Severity: | normal | Keywords: | has-patch has-unit-tests |
| Cc: | Focuses: | performance |
Description
I noticed that on one site wp_normalize_path() was being called about 4,000 times per request. This looks like a good place to add a simple static cache variable. I have put together a small patch to make this happen.
This has the added benefit of also dropping the number of calls to wp_is_stream(). On the test request I was looking at it went from 4,190 to 1,460. While wp_is_stream() doesn't do much, when we are talking about ~4,000 calls even tiny amounts add up quick. This also showed that adding the cache to wp_normalize_path() provides a pretty good hit rate ( hovering around 66% ).
Extracting just those to functions and comparing the uncached vs. cached wp_normalize_path(), at 4,000 calls the time went from 1.4ms to 0.4ms ( PHP 8.4.7 on M3 laptop ).
Change History (36)
This ticket was mentioned in PR #10770 on WordPress/wordpress-develop by @josephscott.
6 months ago
#1
- Keywords has-patch has-unit-tests added
#3
@
6 months ago
- Focuses performance removed
This is a classic "use more memory to reduce I/O and CPU" trade off. At the ~4,000 calls to wp_normalize_path() I was seeing about 240KB additional memory usage. This is of course going to vary depending on exactly what paths you end up normalizing.
#4
@
6 months ago
@josephscott out of curiosity, did you test any variation with a limited cache size to purge out old entries? 240 KB isn’t much, but it’s also not nothing when thinking about requests that can be fulfilled in 16–19 MB total. That’s potentially around a 1% bloat I think.
#5
@
6 months ago
I did not. I'd consider the 240KB for ~4,000 calls to be on the high side of things. In a quick benchmark generated by Opus it had the memory usage coming in about 105KB for 2,000 requests.
#6
@
6 months ago
@josephscott I also noticed that wp_is_stream() starts with strpos( $path, '://' ) and yet it seems like we should have some constraints to limit this, making the worst-case of inputs needlessly inefficient here.
in fact, it looks like there could be significant improvement in that function and I wonder how much of an impact it would have if you applied some optimizations there in your test code.
<?php function wp_is_stream( $path ) { if ( ! is_string( $path ) || '' === $path ) { return false; } // `php`, `file`, `http`, `https`? will always be available, or else things would break… if ( 1 === strspn( $path, 'hfp', 0, 1 ) && ( str_starts_with( $path, 'http://' ) || str_starts_with( $path, 'https://' ) || str_starts_with( $path, 'file://' ) || str_starts_with( $path, 'php://' ) ) ) { return true; } // Valid protocol names must contain alphanumerics, dots (.), plusses (+), or hyphens (-) only. $protocol_length = strspn( $path, '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz.+-' ); if ( 0 === $protocol_length || 0 !== substr_compare( $path, '://', $protocol_length, 3 ) { return false; } return in_array( substr( $path, 0, $protocol_length ), stream_get_wrappers(), true ); }
on the other hand, I don’t know how often we expect stream wrappers to change. the only place in Core I one in use was in a test file, and the plugin directory mostly only shows plugins adding `guzzle` or `sftp`. would it make sense to cache stream_get_wrappers() instead of the paths? I’m not sure if we should generally be passing around those paths anyway or if they are limited internally within the vendorred code.
<?php function wp_is_stream( $path ) { static $known_schemes = null; if ( null === $known_schemes ) { $known_schemes = ' '; foreach ( stream_get_wrappers() as $scheme ) { $known_schemes .= "{$scheme} "; } } // Valid protocol names must contain alphanumerics, dots (.), plusses (+), or hyphens (-) only. $protocol_length = strspn( $path, '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz.+-' ); if ( 0 === $protocol_length || 0 !== substr_compare( $path, '://', $protocol_length, 3 ) { return false; } $scheme = substr( $path, 0, $protocol_length ); return str_contains( $known_schemes, " {$scheme} " ); }
By caching the stream wrappers and eliminating the array code we should up with an extremely fast lookup that stays within a few 32-byte cache lines on most systems.
To summarize:
- how much are we losing performance-wise by looking for the scheme separator at any point in the string vs. anchoring it at the front?
- how much loss comes in through the array functions?
- how much of the overhead is calling
stream_get_wrappers()repeatedly, which, if cached, would not be more stale than caching the$pathresults but would involve considerably less memory cost. (PHP 8.5.2 on my laptop shows 12 schemes of which there are a total of 65 characters).
#7
@
6 months ago
Thanks to some additional ideas from Matthew Reishus I experimented with various approaches to caching here. After a number of iterations I settled on a segmented cache approach. This avoids bringing in a whole new class to do LRU, while still keeping a cap on the size & memory.
This segmented approach still got a 66% cache hit ratio on my large scale test site, with a cap of 100 entries for each segment, but the memory usage was only ~35KB. Seems like a good trade off for an additional two dozen lines or so.
I have updated https://github.com/WordPress/wordpress-develop/pull/10770/changes to use this segmented approach.
#8
@
6 months ago
@dmsnell I tested this version:
function wp_is_stream( $path ) {
static $known_schemes = null;
if ( null === $known_schemes ) {
$known_schemes = ' ';
foreach ( stream_get_wrappers() as $scheme ) {
$known_schemes .= "{$scheme} ";
}
}
// Valid protocol names must contain alphanumerics, dots (.), plusses (+), or hyphens (-) only.
$protocol_length = strspn( $path, '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz.+-' );
if ( 0 === $protocol_length || 0 !== substr_compare( $path, '://', $protocol_length, 3 ) ) {
return false;
}
$scheme = substr( $path, 0, $protocol_length );
return str_contains( $known_schemes, " {$scheme} " );
}
I didn't see any difference in the timing for wp_normalize_path() from the current code.
#9
@
6 months ago
Thanks for testing that out @josephscott — it’s really interesting that the call to stream_get_wrappers() wasn’t the bottleneck, if I understand your results correctly.
Do you think this implies the cost is in the preg_replace() inside of wp_normalize_path()? I guess we are discussing a fairly trivial aspect of performance, even though it’s at startup.
#10
@
6 months ago
My current theory is that preg_replace() is responsible for the bulk of the work. I haven't bothered to narrow it down though, since the simple inline segmented caching seems to provide a nice win.
#11
@
6 months ago
- Focuses performance added
@josephscott great work. this seems a bit non-controversial now, especially with the limited impact of the cache.
linking here for posterity’s sake, I have proposed an update to wp_normalize_path() that removes the PCRE calls and eliminates the call to stream_get_wrappers().
In my little investigation is seems almost like a defect that we are using that function to determine if a path specifies a stream or not, because paths with valid stream schemes/protocols should definitely be retained even if the running PHP instance doesn’t happen to be aware of them.
if a path is provided as git+ssh://github.com/wordpress/wordpress-develop.git, for example, it should not be transformed into git+ssh:/github.com/..., which is what the legacy code does. I think it would be good to consider eliminating the call to wp_is_stream() entirely regardless of what we do.
This ticket was mentioned in Slack in #core-performance by josephscott. View the logs.
6 months ago
This ticket was mentioned in Slack in #core-performance by dmsnell. View the logs.
6 months ago
This ticket was mentioned in PR #10781 on WordPress/wordpress-develop by @dmsnell.
6 months ago
#14
Trac ticket: Core-64538
In addition to optimizing, two changes were introduced:
- The scheme separators for non-registered stream wrappers will be presered. Previously, these were combined into a single slash.
- Windows drive letters which are not found at the start of the string will be upper-cases if they are not. Previously, if a drive letter were found after the first character and was lower-case, it would remain lower-case.
@josephscott commented on PR #10770:
6 months ago
#15
I worked with Opus to address the issues brought up by Copilot.
#16
@
6 months ago
I was looking at https://github.com/WordPress/wordpress-develop/pull/10781 to see what it would do performance wise. After a bit I realized that I was juggling a fair number of variations of this work on wp_normalize_path() and was testing it in a environment with a huge number of moving parts. It felt like a good time to pull back and really re-evaluate what is going on, and to do it in a stand alone way.
Working with Claude I put together a stand alone benchmark script that tries the various approaches and compares them - https://github.com/josephscott/wp-normalize-path - and this turned out to be really helpful.
In these isolated tests the segmented cache did not perform as well. That ultimately led me to have Claude automatically try out variations it could think of and use the benchmark script to see how well they did. That is why the script has other variations in it that are not included in the output.
The original simple cache ended up being a better option than the segmented, original, and the variation @dmsnell has in https://github.com/WordPress/wordpress-develop/pull/10781. But Claude made a few tweaks to the simple approach that gave up a bit of the performance win for less memory usage. It labeled that variation "best".
A few other changes I made - the benchmark doesn't include Windows and UNC paths. I expect the vast majority of WP installs are only dealing with Unix like environments. So I simplified the paths to be tested to focus on that.
Another data point that I found interesting. My practically fresh WP install from wordpress-develop does about 800 calls to wp_normalize_path(). Meaning out of the box this would add less than 60KB to PHP memory and reduce the time spent by ~0.2 seconds ( in my local Apple Silicon M3 test results ).
I'm hoping that by making this public and easy for anyone to use that we can zero in on a final solution. And of course if you find any bugs, or have other variations that should be tested, this is easy to update.
@dmsnell commented on PR #10781:
6 months ago
#17
Measured performance data suggests two slowdowns:
- the
//replacement loop - finding the stream protocol length, which is faster when doing
strcspn( $path, ':' )instead of the positive list.
it does seem that the string-processing is itself the bottleneck.
#18
@
6 months ago
@josephscott this is wonderful — thank you for sharing your benchmark. with it, I was able to investigate a few more things.
- as noted in the comment above, the
while ()loop replacing//seemed to be the heaviest bottleneck. - it seems like all the string processing is indeed the main source of delay. skipping that, via the cache, is exactly what’s lifting the overall performance here.
- with a warm JIT, the cacheless optimization with my two changes above brought it within a little over 2x the runtime of the cached version. the overall performance of the non-cache version was between 25% and 45% faster than the original.
I even wondered about the odd generate_paths() functions and if we could have been skewing the results due to pre-computing the string hashes in generate_paths() outside of the timing function. That is, array lookup is much cheaper if that hash has already been computed. The cached version was still faster, though the gap was reduced when I moved this into the benchmark. In order to facilitate this I also precomputed a large list of paths as a newline-delimited string and had generate_paths() randomly point into that string to grab a line.
you have exhausted all of the cases I can think of, and I wonder how generally this approach could apply to other Core functions which see the same strings frequently. there are probably many out there that do, where a limited cache would serve us better than uncapped or large caches.
the one thing I think is worth reflecting on are the cases where a given path loads before a stream wrapper is registered. there seemed to mostly be a couple stream wrappers in the plugin directory, Guzzle being one of them. I foresee the failure scenario being the case that we normalize a path before the plugin code is activated, and then that path stays in memory.
it does seem low-risk, as one might expect plugin code to run before processing the rest of the request, so as long as those require or require_once statements don’t appear dynamically and on-demand, this should not misreport the paths. regardless, this is what I believe to be a bug anyway, so that point will be moot if we fix the bug, as the actual cached value will then only rely on the normalized path and not on stream_get_wrappers()
#19
@
5 months ago
- Keywords changes-requested added
- Milestone 7.0 → Future Release
Hi there!
Moving this to Future Release as there has been no progress on the ticket for the past few weeks. Feel free to move it back to the milestone once it’s ready for merge.
#20
@
5 months ago
Apologies for the delay and the back and forth on the approach. With all of the tests pointing to the simple static cache as the best performance option at a variety of path counts, I've updated the PR to use the simple static cache method.
As noted by @mukesh27 this clearly isn't going to make it for 7.0, so in the code comments I've aimed for 7.1.0.
#23
@
5 months ago
@westonruter @jonsurrell I think that this ticket can probably go in to 7.0, as it wasn’t delayed for reasons that I think are relevant. if we had been more able to keep up with everything, I think we would have merged it weeks ago.
I’ve added my approval to the PR. Is it still something we could get in, given the history here, or is there a compelling reason to push it back?
I’ve been so distracted by working on fixing what the build change broke that a lot of things have fallen to the side, and I just don’t feel like it’s essential to push this back.
#24
@
5 months ago
- Milestone 7.1 → 7.0
- Owner set to
- Status new → reviewing
@dmsnell Sounds good. You want to commit?
#27
@
4 months ago
- Keywords commit removed
- Resolution fixed
- Status closed → reopened
I noticed that there is one hosting provider who is participating in the distributed host testing program who is experiencing a failure in test_wp_normalize_path_static_cache():
Tests_Functions::test_wp_normalize_path_static_cache Cache should contain the normalized path. Failed asserting that an array has the key '/var/www/cache-testsubdir'. /home/hvofhsd/public_html/tmp/wp-test-runner/tests/phpunit/tests/functions.php:281
It's a bit strange because it passes on PHP 7.4, 8.0, 8.3, and 8.4 but not 8.1 and 8.2.
I have not looked into this deeply, but wanted it to be flagged for investigation before 7.0.
#28
@
4 months ago
that’s odd @desrosj, and it’s interesting to see that the error message has no reverse solidus characters
Failed asserting that an array has the key '/var/www/cache-testsubdir'.
This should read
Failed asserting that an array has the key '/var/www/cache-test\subdir\'.
Now this could be a byproduct of display issues in the log files, but I wonder if it could be related to the platform in any way. The test is dubious, and perhaps I didn’t properly evaluate it in the original ticket, as what matters most is whether the path normalizes as expected; not whether a particular internal optimization runs.
We could try applying this patch to get more info out of the test.
diff --git a/tests/phpunit/tests/functions.php b/tests/phpunit/tests/functions.php
index b6080da780..585e30c0c0 100644
--- a/tests/phpunit/tests/functions.php
+++ b/tests/phpunit/tests/functions.php
@@ -278,7 +278,21 @@ class Tests_Functions extends WP_UnitTestCase {
$static_vars = $reflection->getStaticVariables();
$this->assertArrayHasKey( 'cache', $static_vars, 'Static cache array should exist.' );
- $this->assertArrayHasKey( $path, $static_vars['cache'], 'Cache should contain the normalized path.' );
+
+ $seen_keys = array_keys( $static_vars['cache'] );
+ $seen_keys = array_map(
+ static function ( $key ) {
+ return "'{$key}'";
+ },
+ $seen_keys
+ );
+ $seen_keys = empty( $seen_keys ) ? '(no cached keys)' : implode( ', ', $seen_keys );
+ $this->assertArrayHasKey(
+ $path,
+ $static_vars['cache'],
+ "Path missing from cache: {$seen_keys}."
+ );
+
$this->assertSame( $expected, $static_vars['cache'][ $path ], 'Cached value should match the expected normalized path.' );
}
cc: @josephscott
#29
@
4 months ago
I had Claude code go through the PHP source changes to see if something could be found to explain why this only failed on those two versions of PHP ( 8.1 and 8.2 ). It looks like the way this test works was particularly unlucky.
PHP 8.1/8.2 Bug: ReflectionFunction::getStaticVariables() Returns Stale Values
When OPcache Is Enabled
ReflectionFunction::getStaticVariables() returns compile-time default values
instead of current runtime values on PHP 8.1 and 8.2 when OPcache is enabled.
Affected versions: PHP 8.1.0–8.1.x, 8.2.0–8.2.x (with OPcache enabled)
Not affected: PHP 7.4, 8.0, 8.3+
Cause
Two changes combined to create this bug:
1. PHP 8.1 changed how static variable pointers are stored. In PHP 8.0 and
earlier, the internal static_variables_ptr pointed directly at the function's
own static_variables field, so reflection always read the live data. PHP 8.1
switched to a separate storage slot initialized to NULL, relying on the
ZEND_BIND_STATIC opcode to populate it at runtime.
2. OPcache's Dead Code Elimination (DCE) incorrectly treats ZEND_BIND_STATIC as
side-effect-free for static variables with simple initializers (like static
$cache = array()). This allows the optimizer to eliminate or weaken the opcode,
so the storage slot is never populated. getStaticVariables() then falls back to
returning the compile-time defaults.
PHP 8.0 was unaffected because the self-referencing pointer masked the
optimizer's behavior. PHP 8.3 fixed the issue via the
https://wiki.php.net/rfc/arbitrary_static_variable_initializers
(https://github.com/php/php-src/pull/9301), which introduced a new
ZEND_BIND_INIT_STATIC_OR_JMP opcode that DCE always treats as side-effectful,
and updated DCE to recognize that reference bindings with initializers are
observable through reflection.
Reproduction
<?php
// Run with: php -d opcache.enable_cli=1 repro.php
function demo() {
static $cache = array();
if ( isset( $cache['key'] ) ) {
return $cache['key'];
}
$cache['key'] = 'value';
return $cache['key'];
}
demo();
$statics = ( new ReflectionFunction( 'demo' ) )->getStaticVariables();
// PHP 8.1/8.2 with OPcache: array() (empty — bug)
// All other versions: array( 'key' => 'value' )
var_dump( $statics['cache'] );
References
- https://wiki.php.net/rfc/arbitrary_static_variable_initializers — the PHP 8.3
RFC whose implementation fixed this as a side effect
- https://github.com/php/php-src/pull/9301 — the implementation, including the
DCE fix and the regression test optimize_static_002.phpt ("Keep BIND_STATIC when
static variable has an initializer")
- https://github.com/php/php-src/issues/9177 — related issue about static
variables and reflection
- https://www.npopov.com/2021/10/13/How-opcache-works.html — background on
ZEND_MAP_PTR and immutable functions
I don't have a lot of experience with PHP internals, and given that this is already fixed I think it is more helpful to focus addressing the testing code. What if we split this test into two - https://gist.github.com/josephscott/e1265dc63d878a53947ec6c4cc555390 - one for each group of PHP versions ( with and without this bug ).
For wp_normlize_path() itself, I haven't found anything that needs to be addressed.
#30
follow-up:
↓ 31
@
4 months ago
@josephscott wild.
this appears then to be another case of tests failing.
perhaps instead of splitting the tests, which seems a bit awkward, can we think of a way to assert the end behavior rather than the the method by which the function accomplishes it?
As an aside, when I follow the reproduction steps with PHP 8.1.33 and also with PHP 8.2.29 it works appropriately, so it probably doesn’t affect all of the PHP 8.1 and 8.2 lines. Maybe some hosts haven’t updated to the latest minor releases.
array(1) {
["key"]=>
string(5) "value"
}
Were you successful in reproducing this with the proposed snippet?
#31
in reply to: ↑ 30
@
4 months ago
I didn't run the repo code - I guess it included it in an effort to be "helpful". I took a quick run at it and I'm not seeing the failure. Either the repo example is useless ( entirely possible ) or there is another variable at play that I haven't matched yet.
#32
@
4 months ago
I have been going through variations of PHP versions and haven't been able to reproduce this. Is it possible that this is something specific to the configuration of the host? Do they have a custom build of PHP? At this point I think we need more data.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 months ago
#34
@
3 months ago
So interestingly, the host who was experiencing this failure is no longer reporting an issue.
It looks like the final changeset that reported the problem was [62207]. Interestingly, 62207 tested both a MariaDB and MySQL setup. The test reports for [62208] only reported MySQL usage.
While [62208] was a test-related change, it's not immediately clear whether it would have influenced the occurrence of this issue. But it seems that no other hosts have reported that specific failure and that host has been fixed since.
https://core.trac.wordpress.org/ticket/64538