Optimize template_preprocess() [#660856]

On a front page with 10 article teasers, template_preprocess() is called 40 times (10 times for 'node', 10 times for 'field', 10 times for 'user_picture', 5 times for 'region', 3 times for 'block', once for 'page' and once for 'html').

On a node with 50 comments, template_preprocess() is called 114 times (50 for 'comment', 51 for 'user_picture', and another 13). If #538164: Comment body as field lands, and if the hack that's bypassing field theming is removed from it, that will add another 50 calls, because there's a desire to preserve field.tpl.php as a template and not convert it to a function (#659788-3: [meta issue] theme('field') is too slow, which is inhibiting its more widespread use) if possible.

So, here's a patch that optimizes template_preprocess(). In preliminary benchmarks, I'm seeing a 1% improvement for the 10 node teasers page and a 2% improvement for the node with 50 comments page, but I'd appreciate someone confirming that from another system.

Comment	File	Size	Author
#13	optimize-template_preprocess-13.patch	3.16 KB	effulgentsia
#13
#11	optimize-template_preprocess-11.patch	3.14 KB	effulgentsia
#11
#10	optimize-template_preprocess-10.patch	3.15 KB	effulgentsia
#10
#8	preprocess_before.png	82.36 KB	catch
#8	preprocess_after.png	55.31 KB	catch
#2	drupal-optimize-template_preprocess.patch	3.29 KB	effulgentsia
#2
	drupal-optimize-template_preprocess.patch	3.29 KB	effulgentsia

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

effulgentsia CreditAttribution: effulgentsia commented 16 December 2009 at 01:47

Issue tags:

+Performance

tagging

Comment #2

effulgentsia CreditAttribution: effulgentsia commented 16 December 2009 at 01:57

File	Size
drupal-optimize-template_preprocess.patch	3.29 KB

+++ includes/theme.inc	16 Dec 2009 00:51:02 -0000
@@ -2234,34 +2234,57 @@ function template_preprocess(&$variables
+  if (!isset($default_variables) || ($user != $default_variables['user'])) {

s/!=/!==/

This review is powered by Dreditor.

Comment #3

yched CreditAttribution: yched commented 26 December 2009 at 12:25

Just wondering:

+  $default_variables = &$drupal_static[__FUNCTION__];
+  // Global $user object shouldn't change during a page request once rendering
+  // has started, but if there's an edge case where it does, re-fetch the
+  // variables appropriate for the new user.
+  if (!isset($default_variables) || ($user != $default_variables['user'])) {
+    $default_variables = _template_preprocess_default_variables();
+  }
+  $variables += $default_variables;

Won't that cause alterations on $variables later down the road affect the $default_variables used for other preprocess calls, by the game of var references ?

Comment #4

moshe weitzman CreditAttribution: moshe weitzman commented 28 December 2009 at 00:42

$user != $default_variables['user']

can you compare stdClass objects like that? How is equality determined?

i have a minor, non specific worry about this patch. static caching stuff like db_is_active() seems wrong. but i'm ok with this given that our test suite would catch any big mistake.

Comment #5

effulgentsia CreditAttribution: effulgentsia commented 28 December 2009 at 01:04

Re #3: I don't think so. $default_variables is a reference, but $variables += $default_variables does a by-value copy of the items within $default_variables to $variables.

Re #4: Patch #2 fixed that to !== which is true if the two objects are not the same instance of the same class: http://php.net/manual/en/language.oop5.object-comparison.php.

Comment #8

catch

he/him

English

CreditAttribution: catch commented 2 January 2010 at 09:59

Status:

Needs review

» Reviewed & tested by the community

File	Size
preprocess_after.png	55.31 KB
preprocess_before.png	82.36 KB

I had previously opened #537094: Optimize template_preprocess() (300% speedup) but won't fixed it again due to similar concerns to Moshe, and it being a relatively small optimization over the whole request. However this has additional optimizations I'd not spotted, and was before we had the new drupal_static() pattern, so it looks a lot cleaner. Also I think it's really unlikely that the return value of db_is_active() changes between one call to template_preprocess() and the next during a request, and if it does then something much worse is going on.

I did a before/after comparison on the front page with ten nodes, viewed by a normal authenticated user with only default permissions. This reduces total time spent in 38 calls to template_preprocess() down from 1.72% of the request, to 0.25%. That's less than 1/6th the time, which is great. My previous patch had reduced down from 3% of the request (when called 478 times) down to 1% - so it's twice as fast as that too. Screenshots attached, there's also before/after screenshots on the other issue for addtional data on what this looks like when called more often (i.e. a comment listing).

I'm happy with the answers in #5 to yched and moshe, and this is a very nice improvement which will make field API a lot less costly, so RTBC.

Comment #9

moshe weitzman CreditAttribution: moshe weitzman commented 2 January 2010 at 17:20

OK, I'm +1 for this too. Lets remove the stigma that templates are significantly slower than theme functions.

Comment #10

effulgentsia CreditAttribution: effulgentsia commented 8 January 2010 at 22:53

File	Size
optimize-template_preprocess-10.patch	3.15 KB

re-rolled.

Comment #11

effulgentsia CreditAttribution: effulgentsia commented 9 January 2010 at 10:13

File	Size
optimize-template_preprocess-11.patch	3.14 KB

Oops. #10 was a bad re-roll that failed to take full advantage of the recently committed improvement to the advanced drupal_static() pattern. This one fixes it.

Here's more benchmark summary to complement #8. Specifically, I started this issue to get us to the point where comment body can be themed using theme('field') without it incurring an unacceptable performance penalty, so the benchmark summary here follows the same pattern as on #678714-5: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance for viewing node with 50 comments:

HEAD: 201.1ms
HEAD plus allowing comment body to use theme('field') (i.e., remove the hack in comment_build_content()): 214.3ms (+6.6%)
HEAD plus allowing comment body to use theme('field') plus this patch: 212.1ms (+5.5%)

In other words, once we enable comment body to be themed as a normal field, this patch gives us a net 1% savings. Not all of that is from optimizing the theme('field') call, since this patch also optimizes all calls to template_preprocess(). Also note that the reason for the difference in the above 6.6% number from the other issue's 7.6% number is partially because #667038: Optimize template_process() landed which is already helping theme('field') have a little less overhead. Finally, the savings from here is additive with the one in #678714: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance, so if both land, allowing comment body to be themed with theme('field') would only cost ~4% on a node with 50 comments. More improvements may still be possible: what's our target?