Ticket #3875 (closed defect: fixed)

Opened 2 years ago

Last modified 1 year ago

Proposal for a new plugin architecture

Reported by: FraT Assigned to: ryan
Priority: normal Milestone: 2.2.3
Component: Optimization Version: 2.1.1
Severity: normal Keywords: has-patch plugin plugins plugin.php filters $wp_filter wp-includes
Cc:

Description

I'm proposing a new structure of $wp_filter array.

This one can prevents the loops to add and to remove filters.

With this changes the follow code is exec with not relevant differences:

for($i = 0, $j = 100000; $i<$j; $i++)
	add_filter("tag$i", "funtion");

But this is faster:

for($i = 0, $j = 100000; $i < $j; $i++)
	for($i2 = 0, $j2 = 3; $i2 < $j2; $i2++)
		add_filter("tag$i", "funtion$i2");

The core of my proposal is to change the add_filter function with this one:

function add_filter($tag, $function_to_add, $priority = 10, $accepted_args = 1) {
	global $wp_filter;

	$wp_filter[$tag][$priority][serialize($function_to_add)] = array('function' => $function_to_add, 'accepted_args' => $accepted_args);
	return true;
}

Attachments

plugin.php (3.6 kB) - added by FraT on 02/26/07 11:21:52.
An example of wp-includes/plugin.php modified to the $wp_filter structure that I'm proposing
plugin.php.diff (5.3 kB) - added by markjaquith on 02/26/07 16:09:51.
Previous file as a patch
wp-includes.plugin.php.diff (3.8 kB) - added by santosj on 07/31/07 21:49:41.
Patch for review, hasn't been tested, but might provide fixes.
wp-includes.plugin.php.2.diff (2.4 kB) - added by santosj on 08/01/07 17:33:48.
Fixes issues in previous diff, overwriting.
TestPlugin.php (3.7 kB) - added by santosj on 08/01/07 18:10:57.
Plugin Unit Test
plugin.diff.php (2.0 kB) - added by darkdragon on 08/17/07 06:05:38.
Optimized patch to decrease time by one half with twice as much
testplugin.php (1.5 kB) - added by darkdragon on 08/18/07 01:11:20.
Proof of Concept of Bug and Proof that Patch works! If you get the not so friendly message, then the current plugin.php needs to be patched.
testplugin.2.php (1.5 kB) - added by darkdragon on 08/18/07 01:33:06.
Remove from admin panel too and more kinder words.
wp2_2.plugins.php.diff (1.9 kB) - added by santosj on 08/24/07 18:29:11.
Working patch for WordPress 2.2.x branch. Code is similar.

Change History

02/26/07 11:21:52 changed by FraT

  • attachment plugin.php added.

An example of wp-includes/plugin.php modified to the $wp_filter structure that I'm proposing

02/26/07 16:09:51 changed by markjaquith

  • attachment plugin.php.diff added.

Previous file as a patch

02/26/07 16:13:05 changed by markjaquith

  • keywords changed from plugin plugins plugin.php $wp_filter wp-includes to has-patch 2nd-opinion plugin plugins plugin.php filters $wp_filter wp-includes.
  • owner changed from anonymous to ryan.

Patch uploaded without endorsement. Haven't had time to grok the changes. But as long as it is 100% backwards compatible and is faster, it should be good for inclusion. It sure is less code! What say you, Ryan?

FraT, do you have any numbers comparing the speed differences?

02/26/07 18:01:05 changed by ryan

If it is faster and doesn't break anything, I'm down. Performance numbers would indeed be interesting.

02/27/07 19:24:19 changed by ryan

I did some profiling with xdebug and cachegrind. It seems to be faster and reduces the number of function calls made by apply_filters().

+1

02/27/07 23:22:55 changed by markjaquith

Excellent. You did the research, so you can have the honors.

I like tickets like this... more please!

02/28/07 01:09:22 changed by ryan

  • status changed from new to closed.
  • resolution set to fixed.

(In [4955]) add and apply filter optimizations from FraT. fixes #3875

07/05/07 18:09:14 changed by weyhan

  • status changed from closed to reopened.
  • resolution deleted.

I would like to point out that the change actually did break something.

I have post the details at the forum http://wordpress.org/support/topic/124805?replies=2

Please have a look and let me know if my accessment is correct.

07/05/07 19:55:38 changed by foolswisdom

  • milestone changed from 2.2 to 2.2.2.

07/10/07 15:59:51 changed by daviding

  • priority changed from low to normal.
  • type changed from enhancement to defect.
  • severity changed from minor to normal.

Does this mean that the code will be reverted to as it was before the code was changed?

It may be more efficient, but I really miss using Post Teaser ... which stopped working at Wordpress 2.2.1.

07/21/07 14:52:29 changed by Nazgul

  • keywords changed from has-patch 2nd-opinion plugin plugins plugin.php filters $wp_filter wp-includes to 2nd-opinion plugin plugins plugin.php filters $wp_filter wp-includes.

(follow-up: ↓ 11 ) 07/31/07 17:02:23 changed by santosj

It is simple to fix.

Just check to see that $function_to_add is an array and if it is, then take the first key ( $function_to_add[0] ) and see if it is an object or a string. If it is an object, then get the name ( get_class($function_to_add[0]) ) and use that for the serialized part.

It may decrease the speed a little bit, but it will fix the previous issues. And actually, for plugins and internal code that uses functions, it won't affect the speed at all (or so in my humble opinion).

function add_filter($tag, $function_to_add, $priority = 10, $accepted_args = 1) {
    global $wp_filter;

    $serialized = &$function_to_add;
    if(is_array($function_to_add))
    {
        if(is_object($function_to_add[0]))
        {
            $serialized = array(get_class($function_to_add[0]), $function_to_add[1]);
        }
    }

    $wp_filter[$tag][$priority][serialize($serialized)] = array('function' => $function_to_add, 'accepted_args' => $accepted_args);
    return true;
}

In theory, this should work.

(in reply to: ↑ 10 ; follow-up: ↓ 12 ) 07/31/07 17:06:46 changed by santosj

Well, I suppose the multiple if statements are not needed. If speed is what you are looking for then it should be slightly faster than with both of them. I suppose, since it would fail with the first statement and not evaluate the second once it sees that the two are AND.

Replying to santosj:

function add_filter($tag, $function_to_add, $priority = 10, $accepted_args = 1) {
    global $wp_filter;
 
    $serialized = &$function_to_add;
    if(is_array($function_to_add) && is_object($function_to_add[0]))
    {
        $serialized = array(get_class($function_to_add[0]), $function_to_add[1]);
    }

    $wp_filter[$tag][$priority][serialize($serialized)] = array('function' => $function_to_add, 'accepted_args' => $accepted_args);
    return true;
}

(in reply to: ↑ 11 ) 07/31/07 17:24:00 changed by fedallah

When I replied to the Trac email on this post earlier in the month, I figured the list would thread my response to Trac, too. Oops. So here's a link to the wp-hackers email about this bug for reference:

http://comox.textdrive.com/pipermail/wp-hackers/2007-July/013734.html

Anyway, re: santosj, the get_class() approach fails whenever an object is instantiated more than once, and each instance is attempted to be registered with the same method. I wrote about this in my message to the list earlier this month.

Example code:

class Foobar {
	var $name;
	
	function Foobar( $name ){
		$this->name = $name;
	}
	
	function hook( $v ) {
		return $this->name . " Hooked! $v";
	}
}

$foo1 = new Foobar("baz");
$foo2 = new Foobar("quux");

create_filter( 'the_content', array( &$foo1, "hook" ) );
create_filter( 'the_content', array( &$foo2, "hook" ) );

$foo1 and $foo2 will have the same class name, and therefore, will be the same index in the filter array. So sadly, this solution doesn't work. See my post for one which I think has a better shot.

Also, serialize is big and expensive. I think we should avoid calling it.

07/31/07 18:08:33 changed by santosj

Thanks, I sent a reply to the mailing list.

07/31/07 21:49:41 changed by santosj

  • attachment wp-includes.plugin.php.diff added.

Patch for review, hasn't been tested, but might provide fixes.

08/01/07 17:33:48 changed by santosj

  • attachment wp-includes.plugin.php.2.diff added.

Fixes issues in previous diff, overwriting.

08/01/07 17:47:40 changed by santosj

The new patch is slower than the first patch, but I have been able to confirm that it fixes the issues brought by serialize.

You can improve the speed by moving the code from the _wp_filter_build_unique_id() to the add_filter and remove_filter code. It is only about ~10-13 ms improvement with 2000 add_filter() calls.

08/01/07 18:10:57 changed by santosj

  • attachment TestPlugin.php added.

Plugin Unit Test

08/02/07 14:20:02 changed by santosj

  • keywords changed from 2nd-opinion plugin plugins plugin.php filters $wp_filter wp-includes to has-patch 2nd-opinion plugin plugins plugin.php filters $wp_filter wp-includes.

08/09/07 19:08:57 changed by foolswisdom

  • milestone changed from 2.2.2 to 2.2.3.

08/09/07 19:09:04 changed by foolswisdom

  • milestone changed from 2.2.3 to 2.3 (trunk).

08/17/07 06:05:38 changed by darkdragon

  • attachment plugin.diff.php added.

Optimized patch to decrease time by one half with twice as much

08/17/07 06:10:59 changed by darkdragon

Time Decreased by new patch is as follows:

2000 apply filters with old, wp-includes.plugin.php.2.diff, file took 83 ms total. 4049 apply filters with new, plugin.diff.php, takes ~40 ms total.

serialize method takes less than 20 seconds with 4000 apply filters.

The old method does not work, the new patch does work with classes. Is the small decrease in speed worth not having this patch?

08/18/07 01:11:20 changed by darkdragon

  • attachment testplugin.php added.

Proof of Concept of Bug and Proof that Patch works! If you get the not so friendly message, then the current plugin.php needs to be patched.

08/18/07 01:14:02 changed by darkdragon

Added not so friendly plugin which tests gives the proof of concept for bug and patch. I could make it more friendly, but hey my working copy works fine.

Disclaimer: WordPress does not suck and I'm only being sarcastic. Any hurt feelings, shouldn't not be reflected in words or physical contact.

08/18/07 01:33:06 changed by darkdragon

  • attachment testplugin.2.php added.

Remove from admin panel too and more kinder words.

08/24/07 14:18:08 changed by ryan

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [5936]) Fix how wp_filter array is keyed. Props santosj/darkdragon. fixes #3875 for 2.3

08/24/07 14:21:10 changed by ryan

  • status changed from closed to reopened.
  • resolution deleted.
  • milestone changed from 2.3 to 2.2.3.

We might be able to get this in 2.2.3. Anyone want to port the patch to 2.2?

08/24/07 18:29:11 changed by santosj

  • attachment wp2_2.plugins.php.diff added.

Working patch for WordPress 2.2.x branch. Code is similar.

08/25/07 03:35:46 changed by darkdragon

Testing the new patch, allows the test plugin to function correctly for WordPress version 2.2.2. Code is slightly different than patch for 2.3.

08/25/07 19:07:37 changed by foolswisdom

  • keywords changed from has-patch 2nd-opinion plugin plugins plugin.php filters $wp_filter wp-includes to has-patch plugin plugins plugin.php filters $wp_filter wp-includes.

I was looking at this as a non-severe change to a maintenance release, because the ticket only mentions improved performance. Ryan explained to me that the current 2.2 behavior "breaks quite a few plugins".

08/25/07 20:27:24 changed by darkdragon

Well, it only breaks the ones that choose to use objects. There are ways that objects can get around the limitations with the break. I have recommended in the past that a Registry Pattern would solve the problem.

What is fixed in the patch is the expectation that nothing changed in the class. Normally, this would never be the case. Most plugins that I looked at use the Singleton pattern, which suffers the most.

There are several other fixes that can be applied to the singleton pattern that would also fix this issue ( sleep() method ).

09/03/07 14:59:58 changed by ryan

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [6014]) Fix how wp_filter array is keyed. Props santosj/darkdragon. fixes #3875 for 2.2