I benchmarked a few percent speedup with this simple but a tad bit uuuuugly patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

So this was the other idea mentioned in IRC.

sun’s picture

Also, do we really have to copy the value? I guess not?

+      $function = $info['function'];
chx’s picture

Sorry that idea is broken, see theme_pager() for example.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
892 bytes

Status reset.

sun’s picture

I'm looking at http://api.drupal.org/api/function/theme_pager/7, but do not understand what the problem is. We're passing NULL, so the default value is assigned.

chx’s picture

Nope we are passing NULL so that is assigned.

catch’s picture

node/n ab -c1 -n2000

HEAD:
Requests per second: 9.47 [#/sec] (mean)
Time per request: 105.608 [ms] (mean)

Patch:
Requests per second: 9.59 [#/sec] (mean)
Time per request: 104.324 [ms] (mean)

also we still use call_user_func_array() for preprocess functions elsewhere in theme() - any reason not to give those the same treatment?

moshe weitzman’s picture

I can live with this. Lets add a comment - "call_user_func_array() was benchmarked and deemed too slow for heavy use."

sun’s picture

@catch: Was page caching enabled or 404/403 or fatal error?

If not, you somehow made Drupal 3 times faster since your last benchmarks, and, I definitely would want to know how you achieved that! :)

catch’s picture

@sun - these benchmarks are on node/n with no comments. Looks like the other benchmarks were on /node with 10 nodes (although I appear to have mislabeled at least one round). And yes 9 nodes does make that much difference.

chx’s picture

FileSize
1.41 KB

Killing preprocess is even easier as the number of arguments is fixed.

Crell’s picture

Status: Needs review » Needs work

We should add a default in the switch to fall back to cufa() if we run into a theme function that has lots and lots of parameters. Otherwise, we're hard-coding a maximum, arbitrary number of parameters. Covering the most common cases (5 or fewer params, or some number) is sufficient as long as we have a fallback.

We also still need much better comments on why we're doing something so ridiculous. Just seeing that code on its own, I'd immediately submit a patch to convert it to the shorter cufa() if I hadn't already been working on this issue.

catch’s picture

Status: Needs work » Needs review

node/n (no comments)

HEAD:
Requests per second: 9.89 [#/sec] (mean)
Time per request: 101.087 [ms] (mean)

Patch:
Requests per second: 9.94 [#/sec] (mean)
Time per request: 100.566 [ms] (mean)

I also tested with 300 comments and there was no measurable impact either way (only 100 requests though).

This has the nice side effect of webgrind being able to tell you which functions were called by theme() instead of telling you cufa() then having to find cufa() then finding the theme functions from there. One less step in debug_backtrace() too.

catch’s picture

Status: Needs review » Needs work

Didn't mean to reset status. Better comments and a fallback both sound good.

catch’s picture

FileSize
3.73 KB

Tried some more benchmarks but got failed requests. New benchmarks on a clean HEAD (well upgraded devel content from d6 but clean filesystem).

/node - 10 nodes.
HEAD:
6.65 [#/sec]

Patch:
6.66 [#/sec]

full ab output attached.

Could do with someone else running some on other pages / different system.

dmitrig01’s picture

do this for module_invoke and module_invoke_all and then see if there's a speedup

jrchamp’s picture

FileSize
4.44 KB

The count() switch() method is slower than cufa when the overhead from count() is included. If the number of arguments is already available in the function scope, then switch() is only faster when there are 4 or fewer arguments (in ascending order). If the number of arguments is already available in the function scope and the arguments switch() is in descending order, it outperforms cufa for at least up to 10 arguments (note that zero arguments is marginally worse).

If you don't use the switch() method and hope it will ignore the extra null parameters, there is enough overhead from even two null parameters to make it worse than cufa.

Because count() of $args isn't already available in theme(), cufa is the best method.

The second part of this patch assumes $preprocess_function is a string. If an array is provided (to use a member function or statically defined function) it will not work. However, call_user_func() is somewhat faster than cufa, so you could use that.

I have attached a test script which takes about 15 seconds to run and uses sprintf and an increasing number of arguments as the parameters.

EDIT: Padding with nulls using Crell's $args + array(NULL.......) yields no performance benefit over cufa, and is worse than the unmodified args with more than 7 arguments.

catch’s picture