Make WordPress Core

Opened 14 months ago

Closed 10 months ago

Last modified 10 months ago

#58458 closed enhancement (fixed)

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

Reported by: bor0's profile bor0 Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.3
Component: Plugins Keywords: has-patch has-unit-tests commit
Focuses: performance Cc:

Description

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.

Attachments (5)

before_58458.png (36.7 KB) - added by bor0 14 months ago.
after_58458.png (36.6 KB) - added by bor0 14 months ago.
58458.patch (2.6 KB) - added by bor0 14 months ago.
Screenshot 2023-06-19 at 11.03.43.png (233.8 KB) - added by spacedmonkey 13 months ago.
Screenshot 2023-09-14 at 14.51.52.png (53.9 KB) - added by spacedmonkey 10 months ago.

Download all attachments as: .zip

Change History (30)

@bor0
14 months ago

@bor0
14 months ago

@bor0
14 months ago

#1 @bor0
14 months ago

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

#2 @SergeyBiryukov
14 months ago

  • Milestone changed from Awaiting Review to 6.3

#3 @spacedmonkey
13 months ago

CC @flixos90 as the performance lead.

#4 @spacedmonkey
13 months ago

  • Milestone changed from 6.3 to 6.4

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.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


11 months ago

#6 @oglekler
11 months ago

  • Keywords needs-testing added

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

#7 @hellofromTonya
11 months ago

  • Keywords needs-unit-tests added

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.

#8 @spacedmonkey
10 months ago

@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.

#9 @bor0
10 months ago

@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!

#10 @spacedmonkey
10 months ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

This ticket was mentioned in PR #5200 on WordPress/wordpress-develop by @spacedmonkey.


10 months ago
#11

  • Keywords has-unit-tests added; needs-unit-tests removed

#12 @spacedmonkey
10 months ago

  • Keywords dev-feedback added

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

This ticket was mentioned in Slack in #core-performance by spacedmonkey. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


10 months ago

#15 @flixos90
10 months ago

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.

#16 @spacedmonkey
10 months ago

  • Keywords commit added

#17 @spacedmonkey
10 months ago

@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?

#18 @flixos90
10 months ago

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

#19 @spacedmonkey
10 months ago

Agree. I am going to commit.

@spacedmonkey commented on PR #5200:


10 months ago
#20

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

#21 @mukesh27
10 months ago

  • Keywords needs-testing dev-feedback removed

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

#22 @spacedmonkey
10 months ago

  • Component changed from General to Plugins

#23 @spacedmonkey
10 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


10 months ago

Note: See TracTickets for help on using tickets.