WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 5 months ago

#26640 new enhancement

Performance Increase in apply_filter() (~11%)

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

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 6 years ago.
apply_filters.2.patch (1.9 KB) - added by dshafik 5 years ago.
apply_filters.3.patch (1.9 KB) - added by dshafik 5 years ago.

Download all attachments as: .zip

Change History (19)

#1 @SergeyBiryukov
6 years ago

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

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

#2 @nacin
6 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
6 years ago

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

#4 @dshafik
6 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 6 years ago by dshafik (previous) (diff)

#5 @dshafik
6 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
6 years ago

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

#7 @wonderboymusic
5 years ago

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

#8 @dshafik
5 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 5 years ago by dshafik (previous) (diff)

#9 @markoheijnen
5 years ago

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

#10 @dshafik
5 years ago

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

#11 @markoheijnen
5 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
5 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
5 years ago

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

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

#14 @dshafik
5 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
5 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
4 years ago

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

Note: See TracTickets for help on using tickets.