Speed up WordPress hooks addition/removal by ~20%

[bor0]

I present a patch where add_filter/remove_filter/has_filter are sped up by ~20%.

Here's what's changed in the proposed patch:

• Calling a static method than a function is faster - check static-faster.php for proof
• Removed unused arguments, passing less data to the stack - speed should be obvious here
• ​spl_object_id is faster than ​spl_object_hash, as we are not calling ​strpprintf.
  • Sometimes WP_Hook::build_unique_id will return a string (when concatenated), and sometimes an integer, but this should be okay.
• No need to concatenate '::' - speed should be obvious here, fewer strings to concatenate
• If the argument is an object, return spl_object_id right away rather than constructing a pair where the second element is '' and then concatenate that empty string - speed should be obvious here
• Bail early on add_filter if $idx is null - speed should be obvious here

Tested with PHP 8.2.5 by launching wp shell a lot of times, after adding mu-plugin.php to the mu-plugins folder.

With the patch applied, several runs average 0.19570341110229492. Without the patch, the runs average 0.24287629127502441. Calculating abs(0.24287629127502441 - 0.19570341110229492)/0.24287629127502441 we get a 19.57% improvement.

[bor0]

PoC

[bor0]

Test code

[bor0]

Remove deprecated function from plugin.php

[bor0]

Add polyfill for spl_object_id - props @nbachiyski for catching this. The polyfill may introduce performance issues the other way around for instances with PHP < 7.2

[bor0]

Update phpdoc and fix a parse issue

[SergeyBiryukov]

Thanks for the patch! I would suggest evaluating each optimization separately:

Replying to bor0:

• Calling a static method than a function is faster - check static-faster.php for proof
• Removed unused arguments, passing less data to the stack - speed should be obvious here
• If the argument is an object, return spl_object_id right away rather than constructing a pair where the second element is '' and then concatenate that empty string - speed should be obvious here

This makes sense to me.

• ​spl_object_id is faster than ​spl_object_hash, as we are not calling ​strpprintf.

If a polyfill is needed for PHP < 7.2, this might have to wait until the minimum required version is bumped in #57345, otherwise it may negatively affect performance on those older versions. See comment:12:ticket:58206 and comment:24:ticket:58012 for similar recent discussions.

• Sometimes WP_Hook::build_unique_id will return a string (when concatenated), and sometimes an integer, but this should be okay.

This would need to be explained in the @return tag. Would it be preferable to always cast the result to a string for consistency?

• No need to concatenate '::' - speed should be obvious here, fewer strings to concatenate

I think this is still needed for canonical callback representation, see [24251] / #23265.

• Bail early on add_filter if $idx is null - speed should be obvious here

I might be missing something, but if ::build_unique_id() always returns a string or an integer, when exactly would it be null and how common might that be? This adds a return value to ::add_filter(), which does not currently have one, for a case that is not quite clear to me yet.

[spacedmonkey]

Trunk

[spacedmonkey]

58291.4.patch

[spacedmonkey]

I am seeing around a 1% improvement. But this improvement would scale to the number of filters on the page. More filters, more improvements. It is a nice bump. Good work @bor0

[knutsp]

Replying to SergeyBiryukov:

If a polyfill is needed for PHP < 7.2, this might have to wait until the minimum required version is bumped in #57345, otherwise it may negatively affect performance on those older versions. See comment:12:ticket:58206 and comment:24:ticket:58012 for similar recent discussions.

That was PHP < 8. For < 7.2 it's only about 10%. Then 90%, increasing week by week, may get better performance.

[bor0]

Replying to SergeyBiryukov:

If a polyfill is needed for PHP < 7.2, this might have to wait until the minimum required version is bumped in #57345, otherwise it may negatively affect performance on those older versions. See comment:12:ticket:58206 and comment:24:ticket:58012 for similar recent discussions.

I'm fine to switch back to the hash function and do this when the time comes :)

Though if it's only 10% for < 7.2 as pointed out by @knutsp, I can do more benchmarks and see what the performance cost would really be for those users.

• Sometimes WP_Hook::build_unique_id will return a string (when concatenated), and sometimes an integer, but this should be okay.

This would need to be explained in the @return tag. Would it be preferable to always cast the result to a string for consistency?

We could be consistent, but casting would take an additional toll. The trade-off here is consistency which doesn't really seem to matter (we only care about uniqueness within WP_Hook)

• No need to concatenate '::' - speed should be obvious here, fewer strings to concatenate

I think this is still needed for canonical callback representation, see [24251] / #23265.

I see, thanks for the additional context. However, that fix seems to cover only the first two cases, while the third action still produces a different string. We can re-introduce :: but it still doesn't seem the right choice as it doesn't cover all cases.

/*
$x = array( 'My_Plugin_Main_Class', 'the_content' );
$y = 'My_Plugin_Main_Class::the_content';
$z = '\\My_Plugin_Main_Class::the_content';

class My_Plugin_Main_Class {
	public static function the_content() {
	}
}

var_dump( _wp_filter_build_unique_id( null, $x, null ) );
var_dump( _wp_filter_build_unique_id( null, $y, null ) );
var_dump( _wp_filter_build_unique_id( null, $z, null ) );
*/
string(31) "My_Plugin_Main_Class::the_content"
string(33) "My_Plugin_Main_Class::the_content"
string(34) "\My_Plugin_Main_Class::the_content"

• Bail early on add_filter if $idx is null - speed should be obvious here

I might be missing something, but if ::build_unique_id() always returns a string or an integer, when exactly would it be null and how common might that be? This adds a return value to ::add_filter(), which does not currently have one, for a case that is not quite clear to me yet.

You are correct. We should just return - I followed the same approach from ::has_filter. If we want to avoid bailing out early, I guess we can remove it from ::has_filter as well?

Basically, the function seems to have some holes - could be worth figuring out how to refactor it so that it covers these cases as well:

wp> _wp_filter_build_unique_id( null, null, null );
PHP Warning:  Undefined array key 0 in /usr/local/var/www/wp-includes/plugin.php on line 1000
Warning: Undefined array key 0 in /usr/local/var/www/wp-includes/plugin.php on line 1000
PHP Warning:  Undefined array key 0 in /usr/local/var/www/wp-includes/plugin.php on line 1003
Warning: Undefined array key 0 in /usr/local/var/www/wp-includes/plugin.php on line 1003
=> NULL

I understand null is not expected to be used with add_filter, but bailing out seemed like a reasonable defensive choice.

[SergeyBiryukov]

Replying to knutsp:

If a polyfill is needed for PHP < 7.2, this might have to wait until the minimum required version is bumped in #57345, otherwise it may negatively affect performance on those older versions. See comment:12:ticket:58206 and comment:24:ticket:58012 for similar recent discussions.

That was PHP < 8. For < 7.2 it's only about 10%. Then 90%, increasing week by week, may get better performance.

Yes, you're right, I was having second thoughts about this too :) It should be fine to use spl_object_id() here, with a polyfill for PHP < 7.2.

Replying to bor0:

• Bail early on add_filter if $idx is null - speed should be obvious here

I might be missing something, but if ::build_unique_id() always returns a string or an integer, when exactly would it be null and how common might that be? This adds a return value to ::add_filter(), which does not currently have one, for a case that is not quite clear to me yet.

You are correct. We should just return - I followed the same approach from ::has_filter. If we want to avoid bailing out early, I guess we can remove it from ::has_filter as well?

Thanks for the clarification, tracked this down to the initial implementation of has_filter() in [6320] / #5231.

Yeah, it does not seem necessary there either if ::build_unique_id() can never return a falsey value in normal usage.

Basically, the function seems to have some holes - could be worth figuring out how to refactor it so that it covers these cases as well:

wp> _wp_filter_build_unique_id( null, null, null );
PHP Warning:  Undefined array key 0 in /usr/local/var/www/wp-includes/plugin.php on line 1000
Warning: Undefined array key 0 in /usr/local/var/www/wp-includes/plugin.php on line 1000
PHP Warning:  Undefined array key 0 in /usr/local/var/www/wp-includes/plugin.php on line 1003
Warning: Undefined array key 0 in /usr/local/var/www/wp-includes/plugin.php on line 1003
=> NULL

I understand null is not expected to be used with add_filter, but bailing out seemed like a reasonable defensive choice.

I would say the function should only accept parameters of the documented type. It appears to be primarily intended for the plugin API where it is always called with the correct parameters, and should not be used on its own, so I think the warnings for invalid input would be expected here.

[SergeyBiryukov]

Replying to bor0:

• No need to concatenate '::' - speed should be obvious here, fewer strings to concatenate

I think this is still needed for canonical callback representation, see [24251] / #23265.

I see, thanks for the additional context. However, that fix seems to cover only the first two cases, while the third action still produces a different string. We can re-introduce :: but it still doesn't seem the right choice as it doesn't cover all cases.

Let's keep '::' for now, it addresses the most common case as noted comment:5:ticket:23265, and the unit test added in [1294/tests] would apparently fail without it. Removing it seems like a micro-optimization that probably won't have a big impact compared to the other changes.

It also matches a ​call_user_func() example, though that might not be super relevant here:

// Type 4: Static class method call
call_user_func('MyClass::myCallbackMethod');

[bor0]

Thank you for all the comments and discussions. In the latest patch:

• Re-introduce '::'
• Remove redundant if checks for ::build_unique_id
• Add phpdoc to explain that ::build_unique_id expects a pair, if an array

With the latest patch ran on mu-plugin.php, I get these two average numbers:

• Before: 0.2395233392715454
• After: 0.19285016059875487

Improvement: ~19.48%

I believe the biggest benefit comes from switching spl_object_hash to spl_object_id. If we use spl_object_hash instead with 58291.5.patch, the number averages to 0.23015589714050294 which brings about 3% improvement.

[bor0]

FWIW in WP_Widget_Factory::register we also use spl_object_hash - could do the same improvement there as well, not sure if as part of this issue though.

[bor0]

Hey @SergeyBiryukov 👋 Curious if there's anything else that we need to do here?

[bor0]

The current patch causes some test failures, and also we need to figure out the impact of WP_CLI because it copies _wp_filter_build_unique_id, leaving some potential for backward incompatibility.

[schlessera]

Changing the function into a static method (and potentially breaking BC) is not needed here. Static methods are not generally faster than function calls, the above static-faster.php file is just providing wrong results because it does not correctly benchmark the part of the PHP code it should.

I've adapted the benchmark file to be more correct by adding a warm-up call and by doing more iterations to average out random fluctuations. The result can be seen here: ​https://3v4l.org/A7Blr. So a function call is on average more than twice as fast as a static method call.

Therefore, I suggest to revert the change from function call to static method, as this should get rid of the potential BC break and even further improve the performance.

[swissspidy]

With core dropping support for PHP < 7.2, there is no longer a need to polyfill spl_object_id

[SergeyBiryukov]

#65269 was marked as a duplicate.

[westonruter]

@bor0 Are you interested in refreshing the patch given the latest feedback? Ideally it would be in the form of a pull request.

Replaces spl_object_hash() with spl_object_id() in _wp_filter_build_unique_id() and WP_Widget_Factory, adds an early return for plain object callbacks, removes the unreachable null return, and drops the stale falsey-key guard in has_filter().

Fixes https://core.trac.wordpress.org/ticket/58291

[bor0]

Hey @westonruter, thanks!

I opened this PR: ​https://github.com/WordPress/wordpress-develop/pull/11865

This is a refreshed version of 58291.5 addressing the feedback from comments 7, 12, and 13:

• Keeps _wp_filter_build_unique_id() as a global function in plugin.php rather than moving to a private static method, per @schlessera's finding that global function calls are ~2x faster than static method calls (comment 12). No deprecation, no BC break.
• Drops the spl_object_id polyfill from compat.php since PHP 7.4+ is now required (comment 13).
• Fixes the @return tag to string|int since spl_object_id() returns int for the direct-object path.
• Also updates WP_Widget_Factory::register() and unregister() to use spl_object_id (comment 9).
• Removes the stale if ( ! $function_key ) guard in has_filter() as agreed in comment 6.

[westonruter]

Are there any unit tests which would be relevant to add here?

Benchmark results of the PR in its current state; tested on wordpress-7.0-RC3 using wp-env

┌───────────────┬───────────────────────┬─────────────────────┬─────────────┐
│ Callback type │ Old (spl_object_hash) │ New (spl_object_id) │    Diff     │
├───────────────┼───────────────────────┼─────────────────────┼─────────────┤
│ string        │ 32.19ms               │ 32.73ms             │ ~0% (noise) │
├───────────────┼───────────────────────┼─────────────────────┼─────────────┤
│ closure       │ 50.59ms               │ 37.27ms             │ −26%        │
├───────────────┼───────────────────────┼─────────────────────┼─────────────┤
│ object+method │ 48.44ms               │ 45.56ms             │ −6%         │
├───────────────┼───────────────────────┼─────────────────────┼─────────────┤
│ static        │ 39.66ms               │ 43.10ms             │ ~0% (noise) │
└───────────────┴───────────────────────┴─────────────────────┴─────────────┘

After several iterations, I noticed that static is consistently slower by 2-3 ms.

This is because they go through the new is_callable guard that wasn't there before, but we decided that the safety guard trade-off was worth it.

[bor0]

Added some tests in ​https://github.com/WordPress/wordpress-develop/pull/11865/commits/3d4339f8e340a8938b8f0f50d3ca9b708edfa665

This is because they go through the new is_callable guard that wasn't there before, but we decided that the safety guard trade-off was worth it.

@bor0 It looks like I was wrong. After re-testing with is_callable(), PHPStan still complains:

------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   src/wp-includes/plugin.php
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  1016   Binary operation "." between numeric-string and mixed results in an error.
         🪪  binaryOp.invalid
  1020   Binary operation "." between mixed and '::' results in an error.
         🪪  binaryOp.invalid
  1020   Binary operation "." between non-falsy-string and mixed results in an error.
         🪪  binaryOp.invalid

I reverted back to what you had before in 6f23ca4, and this is now fixed.

If we wanted to use is_callable(), then the condition would have had to be as follows for PHPStan:

if ( ! is_callable( $callback, true ) || ! is_string( $callback[1] ) ) {

As I understand, what you had before was slightly more performant anyway, so I think it's a win-win.

Via 541dc90 I also added a missing null check in WP_Hook::add_filter() for the return value of _wp_filter_build_unique_id(). This fixes:

  91     Cannot access offset string|null on mixed.                                                                                        
         🪪  offsetAccess.nonOffsetAccessible                                                                                              
         at src/wp-includes/class-wp-hook.php:91  

This check was present WP_Hook::remove_filter():

​https://github.com/WordPress/wordpress-develop/blob/0d0c832f0f857ebfeb4f0050202a02fa7230a3a8/src/wp-includes/class-wp-hook.php#L198-L202

It was also present in ::has_filter():

​https://github.com/WordPress/wordpress-develop/blob/0d0c832f0f857ebfeb4f0050202a02fa7230a3a8/src/wp-includes/class-wp-hook.php#L245-L249

But it was missing in ::add_filter().

Alright, thanks! Looks good 🙌

[westonruter]

In 62408:

Plugins: Improve hook performance by using spl_object_id() instead of spl_object_hash() to construct unique IDs.

• Also use spl_object_id() similarly when registering and unregistering classic widgets.
• Improve typing and phpdoc in _wp_filter_build_unique_id(). Return null for malformed callbacks.
• Add tests for _wp_filter_build_unique_id().
• Improve type safety of WP_Hook::add_filter() in case an invalid callback is provided for parity with ::has_filter() and ::remove_filter().

Developed in ​https://github.com/WordPress/wordpress-develop/pull/11865
Follow-up to r46220, r46801, r60179.

Props bor0, westonruter, SergeyBiryukov, schlessera, arshidkv12, knutsp, spacedmonkey, swissspidy.
See #64898.
Fixes #58291.

[SergeyBiryukov]

In 62842:

Plugins: Remove redundant type casting in wp_filter_build_unique_id().

The (object) type casting was preceded by an is_object() check and can be safely removed.

The isset() language construct is enough to check for an array when detecting malformed callbacks, so the (array) type casting is not required.

Removing the type casting results in an additional performance improvement up to ~8% for the function.

Follow-up to [62408].

See #58291, #64898.

This change introduced a regression that shipped in 7.1. Filed as #65919 with a
minimal repro.

The (string) cast on the object branch is correct at the function boundary, but it doesn't survive being used as an array key in WP_Hook: a bare-object callback produces a canonical decimal string ("1", "5292"), which PHP coerces back to int on key assignment. So WP_Hook::$callbacks stores those callbacks under integer keys, whereas through 7.0.x the key was always a string.

$callback = function ( $post_id ) {};

$idx = (string) spl_object_id( $callback ); // exactly as in core after [62408]
var_dump( $idx ); // string(1) "1"  — a string, as intended

$callbacks = array();
$callbacks[ $idx ] = array( 'function' => $callback, 'accepted_args' => 1 );

var_dump( array_keys( $callbacks )[0] ); // int(1) — coerced back to int

The unit tests added here assert the function's return type (string), but not the resulting key type in WP_Hook::$callbacks, which is where the coercion happens. So this wasn't caught.

Details and proposed fixes in #65919.

[westonruter]

Follow-up: #65919

[westonruter]

In 63334:

Plugins: Prevent object IDs from becoming integer array keys.

To improve performance, r62408 replaced spl_object_hash() with spl_object_id() when building the array keys for WP_Hook::$callbacks and for WP_Widget_Factory::$widgets. Even when the spl_object_id() return value is cast to a string to preserve the previous string array keys, PHP automatically converts such numeric strings to int when assigning the array key. This resulted in a back-compat breakage for plugins that expected to use the array keys in string functions, including possible fatal errors when using strict_types.

To preserve the previous string key behavior, any array keys using spl_object_id() are now prefixed with a string to prevent coercion to int. This not only fixes the expected array key types when iterating over these arrays, but it also fixes the return type for WP_Widget_Factory::get_widget_key() so it actually returns a string.

In addition to the added tests, the non-decimal-int-string type in PHPStan is leveraged instead of a plain string to prevent this regression from returning.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/13209.
Follow-up to r62408, r62733.

Props westonruter, dugi-digitaly, sergeybiryukov.
See #58291.
Fixes #65919.

[westonruter]

In 63576:

Plugins: Prevent object IDs from becoming integer array keys.

To improve performance, r62408 replaced spl_object_hash() with spl_object_id() when building the array keys for WP_Hook::$callbacks and for WP_Widget_Factory::$widgets. Even when the spl_object_id() return value is cast to a string to preserve the previous string array keys, PHP automatically converts such numeric strings to int when assigning the array key. This resulted in a back-compat breakage for plugins that expected to use the array keys in string functions, including possible fatal errors when using strict_types.

To preserve the previous string key behavior, any array keys using spl_object_id() are now prefixed with a string to prevent coercion to int. This not only fixes the expected array key types when iterating over these arrays, but it also fixes the return type for WP_Widget_Factory::get_widget_key() so it actually returns a string.

In addition to the added tests, the non-decimal-int-string type in PHPStan is leveraged instead of a plain string to prevent this regression from returning.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/13209.
Follow-up to r62408, r62733.

Reviewed by adamsilverstein.
Merges r63334 to the 7.1 branch.

Props westonruter, dugi-digitaly, sergeybiryukov.
See #58291.
Fixes #65919.