WordPress.org

Make WordPress Core

Opened 13 years ago

Closed 12 years ago

#3875 closed defect (bug) (fixed)

Proposal for a new plugin architecture

Reported by: FraT Owned by: ryan
Milestone: 2.2.3 Priority: normal
Severity: normal Version: 2.1.1
Component: Optimization Keywords: has-patch plugin plugins plugin.php filters $wp_filter wp-includes
Focuses: 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 (9)

plugin.php (3.6 KB) - added by FraT 13 years ago.
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 13 years ago.
Previous file as a patch
wp-includes.plugin.php.diff (3.8 KB) - added by santosj 12 years ago.
Patch for review, hasn't been tested, but might provide fixes.
wp-includes.plugin.php.2.diff (2.4 KB) - added by santosj 12 years ago.
Fixes issues in previous diff, overwriting.
TestPlugin.php (3.7 KB) - added by santosj 12 years ago.
Plugin Unit Test
plugin.diff.php (2.0 KB) - added by darkdragon 12 years ago.
Optimized patch to decrease time by one half with twice as much
testplugin.php (1.5 KB) - added by darkdragon 12 years ago.
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 12 years ago.
Remove from admin panel too and more kinder words.
wp2_2.plugins.php.diff (1.9 KB) - added by santosj 12 years ago.
Working patch for WordPress 2.2.x branch. Code is similar.

Download all attachments as: .zip

Change History (34)

@FraT
13 years ago

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

@markjaquith
13 years ago

Previous file as a patch

#1 @markjaquith
13 years ago

  • Keywords has-patch 2nd-opinion filters added
  • 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?

#2 @ryan
13 years ago

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

#3 @ryan
12 years ago

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

#4 @markjaquith
12 years ago

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

I like tickets like this... more please!

#5 @ryan
12 years ago

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

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

#6 @weyhan
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#7 @foolswisdom
12 years ago

  • Milestone changed from 2.2 to 2.2.2

#8 @daviding
12 years ago

  • Priority changed from low to normal
  • Severity changed from minor to normal
  • Type changed from enhancement to defect

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.

#9 @Nazgul
12 years ago

  • Keywords has-patch removed

#10 follow-up: @santosj
12 years ago

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.

#11 in reply to: ↑ 10 ; follow-up: @santosj
12 years ago

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;
}

#12 in reply to: ↑ 11 @fedallah
12 years ago

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.

#13 @santosj
12 years ago

Thanks, I sent a reply to the mailing list.

@santosj
12 years ago

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

@santosj
12 years ago

Fixes issues in previous diff, overwriting.

#14 @santosj
12 years ago

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.

@santosj
12 years ago

Plugin Unit Test

#15 @santosj
12 years ago

  • Keywords has-patch added

#16 @foolswisdom
12 years ago

  • Milestone changed from 2.2.2 to 2.2.3

#17 @foolswisdom
12 years ago

  • Milestone changed from 2.2.3 to 2.3 (trunk)

@darkdragon
12 years ago

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

#18 @darkdragon
12 years ago

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?

@darkdragon
12 years ago

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.

#19 @darkdragon
12 years ago

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.

@darkdragon
12 years ago

Remove from admin panel too and more kinder words.

#20 @ryan
12 years ago

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

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

#21 @ryan
12 years ago

  • Milestone changed from 2.3 to 2.2.3
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

@santosj
12 years ago

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

#22 @darkdragon
12 years ago

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.

#23 @foolswisdom
12 years ago

  • Keywords 2nd-opinion removed

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

#24 @darkdragon
12 years ago

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

#25 @ryan
12 years ago

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

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

Note: See TracTickets for help on using tickets.