Move the load of `array_keys` to a different level in the `WP_Hook` class

[bor0]

Since apply_filters is one of the most commonly called functions, it makes sense to not call array_keys every time there, rather, pre-compute it and use it only when needed.

With this patch, we reduce the number of calls by at least 500 (on a basic WP installation) but may scale up depending on plugins, etc. Thereby, improving the performance, if only slightly.

[bor0]

cc @SergeyBiryukov here as well - thank you for your swift responses so far! :)

[spacedmonkey]

CC @flixos90 as the performance lead.

[spacedmonkey]

My profiles, I was seeing less than 0.2ms of compute is used for array_keys. This is an improvement, but a very small one. Moving to 6.4.

[oglekler]

This ticket was discussed during a bug scrub, an audit is needed of where else $this->callbacks is changed to ensure the cached value is refreshed for each change.

Please pay attention that this is an enhancement and needs to be in trunk before Beta 1.

Add props to @hellofromTonya

[hellofromTonya]

The reasoning for the change makes sense to me, even if the performance bump is very very very tiny.

Let's make sure all instances of where WP_Hook::$callbacks is changed does indeed refresh the keys property / cache.

I'm also adding the needs-unit-test keyword to flag checking the current tests to ensure there's coverage for all of the areas being changed in this ticket.

[spacedmonkey]

@bor0 Changes seems valid to me. Is there any chance you could add some unit tests? So not, I would be willing to put some together.

[bor0]

@spacedmonkey thanks for the check-in. I'm overwhelmed right now and likely will be until December at least. If you could help with tests that would be great - I can help review. Thanks!

Trac ticket: https://core.trac.wordpress.org/ticket/58458

[spacedmonkey]

@bor0 I have added unit tests in this PR if you want to take a look.

[flixos90]

Sharing some benchmarks for the PR ​https://github.com/WordPress/wordpress-develop/pull/5200:

• For a basic site with the initial content, the performance difference is very small but seems consistent:
  • wp-total metric with TT3 theme: 94.29ms instead of 95.12ms (0.9% faster)
  • wp-total metric with TT1 theme: 55.5ms instead of 55.97ms (0.8% faster)
• While the difference is so small that it could be variance, using a more synthetic edge case example confirms that this change brings a performance benefit:
  • Running a single filter hook with 100k basic callbacks takes 14.87ms instead of 18.18ms (18.2% faster)

All benchmarks were using npm run research -- benchmark-server-timing -u http://localhost:8889 -n 100 (see ​https://github.com/GoogleChromeLabs/wpp-research), and the Server-Timing implemented in the Performance Lab plugin.

For the edge case benchmark, I added this code:

add_action(
        'init',
        static function () {
                for ( $i = 0; $i < 100000; $i++ ) {
                        $f = function( $value ) {
                                return $value;
                        };
                        add_filter( 'my_test_filter', $f );
                }

                perflab_server_timing_register_metric(
                        'my-test-filter',
                        array(
                                'measure_callback' => static function( $metric ) {
                                        $metric->measure_before();
                                        apply_filters( 'my_test_filter', 'filtered-content' );
                                        $metric->measure_after();
                                },
                                'access_cap'       => 'exist',
                        )
                );
        }
);

Overall, while the real-world impact may differ, it is safe to say this is a worthwhile performance improvement.

[spacedmonkey]

@flixos90 I was doing a final review before commiting and noticed something. See blackfire profile compared ​https://blackfire.io/profiles/compare/01426068-1e58-45c7-b2bc-4df66ffd19f9/graph.

I am seeing i/o and cpu improves ( less than 1% ), but I am seeing an increase of memory usage, around 2%. Around 155kb more memory is used. I am guessing this because of instead of recomputing the array_keys they are now stored. Is this an issue?

[flixos90]

Thanks @spacedmonkey. Given the actual performance sees a small improvement and the memory increase is small enough, I think that's an okay tradeoff.

[spacedmonkey]

Agree. I am going to commit.

@bor0 I have updated the unit tests if you want to take a look before final review.

[mukesh27]

@spacedmonkey The PR got enough approval, ready for commit.

[spacedmonkey]

In 56609:

Plugins: Store result of call to array_keys, to save repeated calls in WP_Hook class.

In the WP_Hook class the function array_keys was called every time an array of hook priorities was needed. For sites with lots of filters or actions, this would result in thousands of calls to the array_keys function, which uses server resources. Instead of recomputing this array every time it is needed, only compute it when filters are added and removed, then store the result as a class property. Improve unit tests to ensure this behaviour is tested.

Props spacedmonkey, bor0, flixos90, hellofromTonya, mukesh27.
Fixes #58458.

Committed https://core.trac.wordpress.org/changeset/56609