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