[wp-trac] [WordPress Trac] #58291: Speed up WordPress hooks addition/removal by ~20%

[WordPress Trac]

#58291: Speed up WordPress hooks addition/removal by ~20%
--------------------------+------------------------------
 Reporter:  bor0          |       Owner:  (none)
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  Awaiting Review
Component:  Plugins       |     Version:  trunk
 Severity:  normal        |  Resolution:
 Keywords:  has-patch     |     Focuses:  performance
--------------------------+------------------------------

Comment (by bor0):

 Replying to [comment:2 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.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/58291#comment:5>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform