WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 7 years ago

#5338 closed enhancement (fixed)

Performance Tweaks for WordPress Trunk Plugin API

Reported by: darkdragon Owned by:
Milestone: 2.5 Priority: normal
Severity: normal Version: 2.5
Component: Optimization Keywords: plugins.php phpdoc has-patch needs-testing
Focuses: Cc:

Description

Clean up Plugin API functions and some optimization tweaks based on WP-Hackers discussion.

  • Use Foreach instead of while loops
  • Move 'all' hook call to its own function
  • Set current filter tag variable after the check for the hook's existence.
  • Remove merged_filters variable

Attachments (2)

plugin.r6328.patch (7.8 KB) - added by darkdragon 11 years ago.
Based off of r6328 for the Plugin API
plugin.r6334.patch (14.5 KB) - added by darkdragon 11 years ago.
Optimizations with PHPDoc updates

Download all attachments as: .zip

Change History (24)

@darkdragon
11 years ago

Based off of r6328 for the Plugin API

#1 @Quandary
11 years ago

The foreach setup breaks in this implementation, because the priority array is not pre-filled; you'll get a bug if, e.g., a pri-1 callback adds a pri-2 callback on the currently-executing hook, and there are no pri-2 callbacks yet. Please see the second patch attached to bug #5335.

I'm also pretty sure that filters (as implemented for apply_filters) are applicable to strings only. do_action is more generic, but it has a completely separate implementation for that.

#2 @darkdragon
11 years ago

I can't and will not advocate something so hackish. What about plugins that hook into at priority of '11' or '12'? Plugins that want to execute after everything else would want to hook into priority of 11, as I have done before with 'init'.

If that is the case, then everything else is pointless and should be reverted back, since it has been known to work.

#3 @Quandary
11 years ago

... because the documentation specifically states that priority levels are 1-10. Anyone doing otherwise is broken.

#4 @darkdragon
11 years ago

Yeah, then it should lock those numbers in, which it doesn't. It allows for any number, therefore unofficially, any priority number must be supported. The documentation states that to keep things standard, but without checks in the code, then it really is just an suggestion on what the plugin authors should do and not what they must do. If it was something they must do, then an error would have been thrown.

The point is, if I hook into 11, then I mostly know that it will be processed last and if I hook into '0' I can guess that it will be run before anything else.

There is no point arguing this, either we add in the checks or we don't, but unless we do, then filling the priority list manually is not an option.

#5 follow-up: @Quandary
11 years ago

You also need to pre-define it if you take out the array sort (I don't see any sorting being done on $wp_filters anymore). Otherwise, the priorities will get all screwed up. Pri-10 will likely wind up running first, because it'll usually be the first level created.

<?php

$example = array();
$example[10] = "This is supposed to be last";
$example[1] = "This is supposed to be first";

foreach($example as $text)
    echo "$text\n";

?>

Output

This is supposed to be last
This is supposed to be first

#6 in reply to: ↑ 5 @darkdragon
11 years ago

  • Cc plugins.php phpdoc has-patch needs-testing removed
  • Keywords plugins.php phpdoc has-patch needs-testing added

Look at the taxonomy. Apply filters are used on objects. Therefore the documentation is correct.

Replying to Quandary:

You also need to pre-define it if you take out the array sort (I don't see any sorting being done on $wp_filters anymore). Otherwise, the priorities will get all screwed up. Pri-10 will likely wind up running first, because it'll usually be the first level created.

<?php

$example = array();
$example[10] = "This is supposed to be last";
$example[1] = "This is supposed to be first";

foreach($example as $text)
    echo "$text\n";

?>

Output

This is supposed to be last
This is supposed to be first

This the reason I had keywords 'needs-testing', I had thought this is the case. Without testing, it is unknown whether this would be the case. I will submit a new patch that fixes this.

#7 @darkdragon
11 years ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Milestone changed from 2.4 to 2.5

#8 @filosofo
11 years ago

Replying to Quandary:

... because the documentation specifically states that priority levels are 1-10. Anyone doing otherwise is broken.

Where do you see that? Even the default filters in wp-includes/default-filters.php have much higher numbers than that.

#9 @Quandary
11 years ago

Wiki. Apply_filter does not have a corresponding limit. This should go to a new bug; it's tangential to the patch.

#10 @ryan
11 years ago

We've documented 1 - 10 in the past, but it was never enforced in code and some places in WP use values like 15, 20, 25, and 50. We could probably go to a 0 - 15 scale and set values > 15 to 15. I think 0-15 would cover most current uses. The priorities used in default-filters can be changed to fall in this range. There's no reason for priorities as high as 50.

#11 @Viper007Bond
11 years ago

The cap can't be 10 as that'd break tons of plugins. Many, many of my plugins need to run after wpautop() slaughters the post for example.

Personally, I think a cap of 20 would be best. This would put the default of 10 right in the middle and allow for enough post-10 room to run enough filters to do most anything in the right order.

#12 @darkdragon
11 years ago

I think no cap is best, since this is sounding like a WTF (Worse Than Failure). The story about the guy who kept trying to check the database connection and try to reconnect, up to 5000 times.

I don't think the wiki should be updated. Adding the checks in might hinder plugin developers and would only slow down the Plugin API. I think Plugin developers should give be given faith in that what they've written is what they meant and not mangle and slap their hand when we feel they've done something wrong.

#13 @Viper007Bond
11 years ago

Yeah, I too personally prefer the flexibility of no limit, but then again, you don't really need 10000 filters/actions.

I think it's worth imposing a cap only if it gives us a performance increase and if that's the case, see my previous comment about the suggested cap.

#14 follow-up: @ryan
11 years ago

We need to profile it to see if its worthwhile. Is losing the need to sort the priorities worth it?

#15 in reply to: ↑ 14 @darkdragon
11 years ago

Replying to ryan:

We need to profile it to see if its worthwhile. Is losing the need to sort the priorities worth it?

No.

@darkdragon
11 years ago

Optimizations with PHPDoc updates

#16 @darkdragon
11 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

I don't know if you want the PHPDoc stuff, I'll remove it, but it does improve upon some of the stuff from the other bugs (#5163 and #5225). Unintentionally, since I've used the one for the PHPdoc checkout instead of the other checkout (I've so many now, I can't keep track of which ones are broken and what I've used them for).

Basically, it is important since there is a merged filters bug in the do_actions (the $merged_filters isn't being set to global, therefore the test will always fail and decrease performance. Won't break anything, but could slow things down by sorting the array each time.)

I've removed the '@' from the current filter assignment, which should speed things up, since '@' is slow until latest PHP 5.2 version (5.2.3 or 5.2.4 has the performance improvement).

I moved the all tag call to its own function and updated all functions to call it instead. The function has PHPDoc info. You will notice that the hook is not part of the call, I figure using the current_filter function is better.

  • I did not use Foreach instead of the do while.
  • I did not remove merged_filters
  • I did not move current_filters assignment.

All in all, I've only did two of the five things in the WP-Hackers post. The rest were premature.

#17 @darkdragon
11 years ago

  • Summary changed from Preformance Tweaks for WordPress Trunk Plugin API, plugin.php to Preformance Tweaks for WordPress Trunk Plugin API

#18 @darkdragon
11 years ago

  • Summary changed from Preformance Tweaks for WordPress Trunk Plugin API to Performance Tweaks for WordPress Trunk Plugin API

#19 @darkdragon
11 years ago

Oh yeah, I use ksort instead of uksort to speed things up a bit (might be premature). I figure it is an edge case that plugin authors will use strings instead of numbers and anyway, it is unknown anyway based on that.

#20 @ryan
10 years ago

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

(In [6361]) Filter and action optimizations and phpdoc updates from darkdragon. fixes #5338

#21 @darkdragon
10 years ago

  • Milestone changed from 2.5 to 2.4

#22 @scribu
7 years ago

Related: #9968

Note: See TracTickets for help on using tickets.