If we do function_exists() || drupal_function_exists() we shave about 10% off the cost of calling theme(), only about .3% of the request on the front page but there's a lot of pages with more theme() calls than that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

FileSize
431.23 KB
430.37 KB
moshe weitzman’s picture

This is really micro-optimization and not desireable, IMO. Lets see what others think. If we want to speed up theme() lets break it up a little so xdebug can guide us better on what parts are chewing time.

FiReaNGeL’s picture

Personally I think that every optimization is good if it doesn't break anything, or uglifies code to a point its not understandable. 10% off a function that is called relatively often per page, why not? As catch pointed out in another thread, all the 'micro optimization' patches shaved off 7% of php time, which is not insignificant. And he just started... catch have all my support!

catch’s picture

Cross-posting since this is on my list of 'things the registry makes slower' #497118: Remove the registry (for functions).

catch’s picture

Issue tags: +Registry
Crell’s picture

Status: Needs review » Needs work

We definitely need to refactor the theme system to find ways to speed it up, but I don't think we can reasonably get away with that this late in the dev cycle. This is a small but dead-simple optimization along the critical path, so I'm OK with it. We should document that fact, however. A single line comment should be sufficient.

catch’s picture

Status: Needs work » Needs review
FileSize
1.5 KB

Added a comment. On micro-optimization, 0.3% is 1/300th of page execution time, for around 30 characters of code.

ugerhard’s picture

Status: Needs review » Reviewed & tested by the community

I feel bold :-)

The patch is "harmless" and brings a performance gain. What's not to like?

catch’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.59 MB
434.66 KB
468.77 KB

More aggressive version - since a lot of theme functions get called several times on the same page, maintain a static of which functions we've already checked.

Attached webgrinds of HEAD + both patches.

catch’s picture

FileSize
2.77 KB
moshe weitzman’s picture

Don't you think PHP itself keeps an in memory array of which functions exist. Why duplicate that with a static? Does its static system work faster?

catch’s picture

As far as I can tell (and I've only looked at the cachegrinds, not tried to go through PHP internals or done a lot of research), the difference in most of these patches is that isset() empty(), if, and static aren't function calls, and function_exists(), drupal_static(), defined() etc. are. So it's the cost of calling a function, vs. not calling a function - regardless of how fast the actual function being called is.

The most obvious example of this is #523058: Optimize check_plain() where it /only/ removes a function call without replacing it with anything else, and takes half the cost of check_plain() out, but I think we can assume that static, empty() etc. are more or less unmeasurable, whereas function calls are measurable (at least in webgrind, if not always ab), once you get over 20-30 of them during a request.

We could probably verify the actual speed improvements better with microbenchmarks over 10,000 iterations or something to see exactly how much difference there is, I only have a certain amount of confidence in webgrind's ability to capture this (although it's very consistent). But I'm not really set up to do those, whereas webgrind screenshots are about two minutes.

Status: Needs review » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
2.51 KB

New patch:

  • "drupal_function_exists()" replaced with "function_exists() || drupal_function_exists()" in two places within theme().
  • slow and unnecessary call_user_func_array() replaced with faster $template_function().
  • While I was in there, my editor stripped trailing whitespace from entire file, even lines unrelated to above.

Benchmarks using attached test523682 module, navigating to test523682/1 (us = microseconds):

  • Calling a fast theme function directly (without using theme()), averaged over 1,000,000 calls: 0.79us
  • Calling a fast theme function using unpatched theme(), averaged over 1,000,000 calls: 11.04us
  • Calling a fast theme function using patched theme(), averaged over 1,000,000 calls: 9.48us
  • Calling a fast theme template using unpatched theme(), averaged over 1,000 calls: 101.5us
  • Calling a fast theme template using patched theme(), averaged over 1,000 calls: 90.7us

Above tests were on a MacBook (Core 2 Duo, 2.0GHz, MAMP with PHP 5.2.6 and APC).

Comments:

  • theme() adds a lot of overhead over calling the function directly. Most of that is due to a slow call_user_func_array() which this patch does not address. However, some of it is due to drupal_function_exists(), and this patch removes about 15% of that overhead.
  • I tried catch's more aggressive patch, but I did not find this to improve the result. In fact, it resulted in worse performance than the simple use of function_exists(). At least on my machine, function_exists() appears to be fast, and trying to get around it through the use of a static array, resulting in an extra setting of a variable and call to isset() does not appear to result in better performance.
  • Calling theme() on a template-implemented hook results in A LOT of overhead. I don't know how much of this is because of the include() within theme_render_template(), and how much of it is from calling the template_preprocess() and template_process() functions. A tiny amount of that overhead (the same 1.5us) can be shaved with the function_exists() optimization, and another 8us can be shaved by replacing the use of call_user_func_array() to invoke the preprocess and process functions with a direct call to those functions, since we know their signature.
  • The effect of these optimizations on real-world page requests depends on what the page request does, and for many page requests this optimization has no meaningful effect. But, I don't see a down-side for applying these optimizations, and perhaps for some page requests, it will actually be useful.
effulgentsia’s picture

Here's a patch that in addition to the above, also addresses the slow call_user_func_array() for function-implemented theme hooks. It's not pretty, but it's commented, and it shaves almost another microsecond (8.66us instead of 9.48us, see benchmarks in comment #16).

@catch: in case you haven't been following #400292: Allow preprocess functions for theme hooks implemented as functions, there's a patch there awaiting someone to allay merlinofchaos's concerns that the feature doesn't add more overhead than the value the feature provides (see comments 7 and 37). Part of what I would like to accomplish with the optimizations here is to more easily justify the extra call to isset() that's needed in that patch. You're trusted as someone who can verify the benchmarks I'm claiming. Please comment on both of these issues. Thanks!

catch’s picture

effulgentsia: these changes look good - could you confirm you're running all the comparisons with xdebug disabled? That caught me out a few times.

effulgentsia’s picture

confirmed. no xdebug. baseline MAMP install with APC turned on and memory_limit at 256M. no other customizations.

effulgentsia’s picture

apologies. i did have zend debugger extension loaded. turned it off and am re-running tests. will post when done.

effulgentsia’s picture

same results with zend debugger extension not loaded.

effulgentsia’s picture

Re-rolled due to #400292: Allow preprocess functions for theme hooks implemented as functions being committed. This patch also moves where "$hook_clone = $hook" is being done before calling preprocess/process functions to more accurately match what was previously being done with "$args = array(&$variables, $hook)".

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Needs review

Oh, testing bot...

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

OK so drupal_function_exists() is dead, which means we're left with the call_user_func_array() optimization. Would be interesting to test that on its own anyway.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
3.02 KB

Re-rolled patch to only include:

  • Replace call_user_func_array() with switch/case for function-implemented theme hooks. This is the only change that affects the below benchmarks.
  • Replace call_user_func_array() with direct function call for preprocess/process functions on template-implemented theme hooks. No benchmarks here for that, but see comment #16. Besides, this is just better, even if the performance gain is minimal.
  • Moved where $hook_clone was being set prior to calling preprocess/process functions on function-implemented theme hooks. While not an optimization in itself, I'm including it with this patch so that there's symmetry between function-implemented and template-implemented theme hooks in this regard.

Attached theme.php for testing. This is a modification of catch's one from http://drupal.org/node/400292#comment-1945102. This modified version tests:

  • theme('username') - a 1 argument theme hook that has a moderately intensive implementation
  • theme('placeholder') - a 1 argument theme hook that has a very light implementation
  • theme('table') - a 6 argument theme hook

Each one is called in 10 sets of 10,000 iterations each. 10 sets in order to calculate a standard deviation. The resulting number is how many microseconds for a single call (i.e., 100 microseconds = 10,000 iterations in 1 second).

Unpatched theme.inc:
nothing__________theme(username)_____theme(placeholder)____theme(table)
0.216 +/- 0.022___100.28 +/- 1.67______16.52 +/- 0.14________165.53 +/- 1.09

Patched theme.inc:
nothing__________theme(username)_____theme(placeholder)____theme(table)
0.216 +/- 0.013___97.75 +/- 0.84______15.22 +/- 0.14________165.34 +/- 1.38

How exciting is a 1-2 microsecond improvement? Maybe not very, but merlinofchaos seems very interested in keeping the theme system overhead as low as possible, so with that in mind, why not commit this?

effulgentsia’s picture

Everything in this issue has been implemented in or made irrelevant by #572618: All theme functions should take a single argument to make preprocess sane and meaningful. What's the right status to set on this? Duplicate? By Design? Somethine Else?

catch’s picture

Status: Needs review » Closed (duplicate)

Duplicate works :)

Dave Reid’s picture

Status: Closed (duplicate) » Needs review

If there's nothing left for this module to do, then just mark as fixed.

effulgentsia’s picture

Status: Needs review » Closed (duplicate)

I suspect #30 was a cross-post with #29. Since catch opened this issue, I'll go with his suggestion. I normally have a tighter internal sense of what "duplicate" and "fixed" mean, and ideally would prefer a new status "no longer relevant". But I'm fine with a looser definition of "duplicate" in the meantime.

Dave Reid’s picture

Status: Closed (duplicate) » Needs review

Sorry about the cross-post. Arg. :)

catch’s picture

Status: Needs review » Closed (duplicate)

;)

catch’s picture

Just a note I like duplicate because it suggests there's a reference to another issue which fixed the original issue - even if the issue that fixed it was completely different.

Fixed I try to reserve for when a patch got committed (but not always).

By design usually means "this is how it works, bad luck", and won't fix is "this is a terrible idea". No longer relevant would be more accurate though.

Dave Reid’s picture

DEAR GOD CROSSPOST Agreed duplicate is a good choice.