Make WordPress Core

Opened 11 years ago

Closed 4 months ago

#26640 closed enhancement (wontfix)

Performance Increase in apply_filter() (~11%)

Reported by: dshafik's profile dshafik Owned by:
Milestone: Priority: normal
Severity: normal Version: 1.5
Component: Plugins Keywords: has-patch
Focuses: performance Cc:

Description

Currently apply_filter() simply calls call_user_func_array(). This is a fairly expensive way to call dynamic functions, especially compared to $foo = 'funcName'; $foo() — the issue is variable arguments.

It is quicker however to do a switch on the number of params and call the equivalent $foo() with a fallback to the old behavior.

Additionally, the loop used is inefficient:

do {
    foreach( (array) current($wp_filter[$tag]) as $the_ )
        ...
} while ( next($wp_filter[$tag]) !== false );

The next() call here returns the same value as current() in the foreach; by simply assigning the return to a variable in the conditional, we remove that extra unnecessary current() call entirely.

} while ( $current = next($wp_filter[$tag]) !== false );

We have to call current() before the do however to assign a value to $current on the first iteration.

Attachments (3)

apply_filters.patch (1.5 KB) - added by dshafik 11 years ago.
apply_filters.2.patch (1.9 KB) - added by dshafik 10 years ago.
apply_filters.3.patch (1.9 KB) - added by dshafik 10 years ago.

Download all attachments as: .zip

Change History (21)

#1 @SergeyBiryukov
11 years ago

  • Component changed from General to Plugins
  • Version changed from trunk to 1.5

Related: [2184], [4955], #17817.

#2 @nacin
11 years ago

  • Component changed from Plugins to General
  • Version changed from 1.5 to trunk

Avoiding current() seems like a nice idea here. I thought for a moment you were going to suggest a switch to foreach, which has its own complications. I think #17817 is the big ticket talking about that.

Switching away from call_user_func_array() has some drawbacks, though. Right now, if a filter's callback does not exist, an E_WARNING is issued. That's just about the right amount of yelling at a developer. See also #15296. $foo(), though, causes a fatal error, which will definitely break sites. I imagine tossing a function_exists() or is_callable() in here would outweigh the switch from call_user_func_array().

#3 @nacin
11 years ago

  • Component changed from General to Plugins
  • Version changed from trunk to 1.5

#4 @dshafik
11 years ago

Added in the function_exists() call with a trigger_error() to emulate the original E_WARNING. It doesn't add noticeable overhead.

Last edited 11 years ago by dshafik (previous) (diff)

#5 @dshafik
11 years ago

So, it was pointed out on Twitter that the callbacks are not always global functions.

This patch will work on PHP 5.4+ where array notation callbacks can be used like $foo() but not in earlier versions, breaking BC.

I'm going to tweak it some more today and see if I can't get a reasonable solution that works in all versions.

#6 @ocean90
10 years ago

  • Focuses performance added
  • Summary changed from [Patch] Performance Increase in apply_filter() (~11%) to Performance Increase in apply_filter() (~11%)

#7 @wonderboymusic
10 years ago

  • Keywords has-patch needs-refresh added
  • Milestone changed from Awaiting Review to Future Release

#8 @dshafik
10 years ago

  • Keywords needs-refresh removed

I have uploaded a new patch, against the latest github master HEAD, this will now do the quickest thing possible based on the version. This will now work in all supported versions of PHP!

  1. When the callback is a simple string, or a Closure, it will always use the faster switch method.
  2. When the callback is NOT a string/Closure, on PHP >= 5.4 it will use the faster switch method, on < 5.4 it'll use the old call_user_func_array() as a fallback.

Possible callbacks include:

  • Strings (global functions)
  • Closures
  • Array containing classname/method (static)
  • Array containing object/method
  • Object instances of classes with an __invoke() magic method

Would love to get this in 4.0.

Last edited 10 years ago by dshafik (previous) (diff)

#9 @markoheijnen
10 years ago

I had to revert the patch while it broke my shortcodes. The rest seemed to work normally.

#10 @dshafik
10 years ago

@markoheijnen hrm, let me spin this up again and double check this. What short codes in particular?

#11 @markoheijnen
10 years ago

This was one I did noticed with: https://gist.github.com/markoheijnen/4236025. I also saw undefined notices with thise line $value = $func($args[1], $args[2], $args[3]); but that was unrelated.

#12 @nacin
10 years ago

It's much too late for this to make it into 4.0 (the possibility of breaking things is extremely high) but I'd definitely be up for considering this in 4.1.

#13 @dshafik
10 years ago

I have uploaded a working patch (apply_filters.3.patch), but I'm re-doing the performance benchmarks.

Last edited 10 years ago by dshafik (previous) (diff)

#14 @dshafik
10 years ago

An update on the benchmarks: the new patch sees a 6% performance increase as opposed to 11%; still quite a big win, though not as big as I'd originally hoped for.

#15 @markoheijnen
10 years ago

That patch works for me. It does still show notices for my widgets for offset 2 and 3 on line 232.

#16 @Tklaversma
9 years ago

Im curious when this be implemented since we're already on 4.2. The same goes for #26638.

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


4 months ago

#18 @johnbillion
4 months ago

  • Resolution set to wontfix
  • Status changed from new to closed

@dshafik I'm sorry that a full decade has passed with no further response to your ticket.

In the meantime the apply_filters() function has seen several performance improvements to its internals, including avoiding the call to array_slice() where possible which was a related bottleneck.

If someone wants to look into whether there is still a performance improvement to be gained here by switching to calling $func rather than call_user_func_array() then we'll need some good benchmarking numbers to accompany it, although I suspect that the addition of the is_callable() check will negate any further micro-optimisations.

Note: See TracTickets for help on using tickets.