Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
I benchmarked a few percent speedup with this simple but a tad bit uuuuugly patch.
Comment | File | Size | Author |
---|---|---|---|
#18 | test21.php.txt | 4.44 KB | jrchamp |
#16 | ab.txt | 3.73 KB | catch |
#12 | switch.patch | 1.41 KB | chx |
#5 | switch.patch | 892 bytes | chx |
#1 | drupal-theme.theme-function.patch | 1.05 KB | sun |
Comments
Comment #1
sunSo this was the other idea mentioned in IRC.
Comment #2
sunAlso, do we really have to copy the value? I guess not?
Comment #3
chx CreditAttribution: chx commentedSorry that idea is broken, see theme_pager() for example.
Comment #5
chx CreditAttribution: chx commentedStatus reset.
Comment #6
sunI'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.
Comment #7
chx CreditAttribution: chx commentedNope we are passing NULL so that is assigned.
Comment #8
catchnode/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?
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedI can live with this. Lets add a comment - "call_user_func_array() was benchmarked and deemed too slow for heavy use."
Comment #10
sun@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! :)
Comment #11
catch@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.
Comment #12
chx CreditAttribution: chx commentedKilling preprocess is even easier as the number of arguments is fixed.
Comment #13
Crell CreditAttribution: Crell commentedWe 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.
Comment #14
catchnode/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.
Comment #15
catchDidn't mean to reset status. Better comments and a fallback both sound good.
Comment #16
catchTried 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.
Comment #17
dmitrig01 CreditAttribution: dmitrig01 commenteddo this for module_invoke and module_invoke_all and then see if there's a speedup
Comment #18
jrchamp CreditAttribution: jrchamp commentedThe 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.
Comment #19
catch#329012: Remove call_user_func_array() from ModuleHandler::invoke() + invokeAll()