[wp-hackers] apply_filters, merge_filters and reset()

[Jacob]

Ryan Boren wrote:
> On 11/9/07, Jacob <wordpress at santosj.name> wrote:
>   
>> Well, I think cleaning the code up would cut some of the branching,
>> which would speed things up. Some things I've noted.
>>
>> 1. Is the key sorting of $wp_filter[$tag] really necessary? I don't
>> think so. Removing the whole merged_filters variable could speed up a
>> portion where apply_filters is first called. I would make a guess and
>> say that most filters are only called once. Which would boost the speed.
>> You would cut the branch (+1), cut out three function calls (reset,
>> uksort, and strnatcasecmp), and a few assignments.
>>     
>
> Getting rid of it would be nice.  I'm digging through svn history
> trying to figure out what the sort was added for.
>   
Well, I'm not sure if you add numbers and it will execute in that order, 
or if I've been adding it in that order and it has been executing in 
that order. In any case, would it make more sense to only do it once for 
each hook? If it doesn't work, then wouldn't it be more wise to call 
ksort($wp_filter, SORT_NUMERIC), instead since it would be quicker.
>   
>> 2. Moving the "Do 'all' actions first into its own function will slow
>> things down, since it is an additional user function call, but having
>> the code in one place would make the code cleaner and any changes
>> easier. The cost would be minimal. You could even keep the branch for
>> checking and put all the rest of the code in a function. That way, the
>> cost for calling the function is only applied if the 'all' hook exists.
>>     
>
> Leaving the check and pushing the rest into a function seems
> reasonable.  We don't need to optimize the all case too much since it
> will only be used in debugging.
>   
The patch does this and pushes the hook name into the parameter list for 
the all functions.
>   
>> 3. @$wp_current_filters[] = $tag is really damn slow. Not the
>> assignment, the '@' character. I don't know what warning or notice it is
>> supposed to suppress, but it would be quicker it is was taken out. If
>> you need to, just check that $wp_current_filter is an array and make it
>> one if it isn't.
>>     
>
> Losing the @ is good.  Actually, now that we pass the action/filter
> tag to the 'all' hooks, there is less reason to keep track of the
> current filter at all.
>   
I did not see where you've done this, but I've added it in another 
parameter for the all hook calling function. You can find the code in 
the patch in the ticket.
>   
>> 4. Have $wp_current_filter[] = $tag; after the check to see if the tag
>> exists. As well as calling the 'all' hook. There is no point of
>> executing everything if it doesn't exist. Although, calling the all hook
>> might be to say that it could test to make sure that the tag has any
>> values, which could be used for debugging, but whether the hook exists
>> or not is not directly passed to the all hook, so the hook would have to
>> test for it on their its own. (using has_filter).
>>     
>
> Okay
>   
What I meant, was that it didn't make sense to append the hook name to 
current filter and then pop it out. Although, what I didn't realize was 
that the all hooks could have called current_filter() to get the current 
hook instead of passing it to the all hooks via a parameter. Therefore 
the all hooks will now have to keep the first parameter as the hook name 
and test all of the rest. This might add additional complexity that 
wouldn't have been needed otherwise. Not to mention it was made 
optional, which might have to be fixed.
>   
>> 5. When is $args ever going to be empty? I know, when the all tag
>> doesn't exist. However, both need it, so it can be placed outside the
>> all hook branch, so that there wouldn't need to be a check for it, which
>> would be executed for about 99.99% of users. Moving it from the branch,
>> would save some time spent executing 'empty' function. (for
>> apply_filters()).
>>     
>
> The idea was to call func_get_args only once and make sure we never
> call it if neither 'all' nor the current tag have hooks set.  I'm fine
> with calling it once for 'all', if set, and once for the current tag
> so that we can skip the empty check.  The extra overhead in the 'all'
> case is no big deal since 'all' is a debugging scenario.  We should
> optimize for the non-'all' case.
>   
I see. Hmm, I still think that the all hooks should only be called if 
the tags exist and not otherwise. This is what would have happened 
before the change. If the filter or action didn't exist then the all 
hooks wouldn't have been called. I think it would be better to keep this 
behavior unless you want to track how many filters and hooks don't have 
hooks.

http://trac.wordpress.org/ticket/5338

-- 

Jacob Santos

http://www.santosj.name - blog
http://wordpress.svn.dragonu.net/unittest/ - unofficial WP unit test suite.

Also known as darkdragon and santosj on WP trac.