One of the slowest parts of field rendering is the call to theme('field'). This patch optimizes it by converting from a template (field.tpl.php) to a function (theme_field), and some other minor optimizations.

On my computer, I'm seeing nearly a 2% improvement on the front page with 10 article teasers:

HEAD: 97.6ms
Patch: 95.9ms

This can make an even bigger difference on a node with 50 comments page once comment body is converted to a field: #538164: Comment body as field.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review

Nice !

We tried this in #438570: Field API conversion leads to lots of expensive function calls (see #12 and #31), but this didn't seem to bring that much of an improvement back then.
Your results seem different, though.

The theme_func version is much less readable than the template, of course. 2% gain is probably a fair tradeoff though.

moshe weitzman’s picture

'mytheme_field__page' refers to the 'page' content type? Lets use 'article' in the example since thats less overloaded than 'page'.

i'm neutral on this one. the speedup is nice but we are degrading the theme DX for our primary D7 innovation.

in the issue that yched links to we discussed a static cache of file_exists() calls in order to speedup template discovery. Not sure if there are any other wins to be had in template themeing. I'm guessing that theme_render_template() is the real albatross there, and I don't any improvements to be had. perhaps a dream, but speeding up template themeing is obviously preferable to ditching field.tpl.php

effulgentsia’s picture

Ok, how about a variant that doesn't change field.tpl.php from being a template, but instead optimizes template_preprocess(), which gets called for all templates, but not for functions.

Also optimizes template_preprocess_field() to not call field_info_instance() and field_info_field().

On my computer, front page with 10 article teasers:
HEAD: 97.9ms
Patch: 96.6ms

More than 1%, so not bad, considering it still leaves it a template. And could be more than 1% for node with 50 comments once comment body is converted to a field.

effulgentsia’s picture

Minor cleanup in this one.

effulgentsia’s picture

Another minor fix.

moshe weitzman’s picture

Not sure that it is safe to do an == comparison on the $user object. See http://www.php.net/manual/en/language.operators.comparison.php#72144.

I'm a bit unsure about this one. Unfortunately, excessive caching is another of my pet peeves. @efflugentsia just can't win. I guess I would prefer this patch over the field.tpl.php removal since the ugliness is hidden from 99% of developers.

effulgentsia’s picture

I split the template_preprocess() optimization to another issue: #660856: Optimize template_preprocess().

This patch has the template_preprocess_field() optimization only, which is not very exciting:

HEAD: 98.0ms
Patch: 97.8ms

moshe weitzman’s picture

I have no idea if the latest patch is a good idea or not. yched?

yched’s picture

+++ modules/field/theme/field.tpl.php	16 Dec 2009 00:51:03 -0000
@@ -13,6 +13,7 @@
  *   CSS. It can be manipulated through the variable $classes_array from
  *   preprocess functions. The default values can be one or more of the
  *   following:
+ *   - field: The current template type, i.e., "theming hook".
  *   - field-name-[field_name]: The current field name. For example, if the
  *     field name is "field_description" it would result in
  *     "field-name-field-description".

I'm not sure I understand the comment. Why do we want to mention "theming hook" here ?

Other than that, the changes look good to me. As effulgentsia stated, that's a much more limited scope and the gains are not spectacular, but it's good cleanup IMO.

This review is powered by Dreditor.

effulgentsia’s picture

I'm not too crazy about that comment either, but it's what we use in every other tpl.php file. See node.tpl.php.

The main thing this patch does that saves a few cycles (admittedly, not many) is remove the calls to field_info_instance() and field_info_field(). And the potential downside of this is that neither additional preprocess functions nor the template file has access to the $field and $instance variables. I don't see preprocess functions or template files in core needing to use these variables, so this seems safe, but we should wait for bot to confirm that (whenever bot wakes up, that is). However, it makes sense to ask whether we foresee contrib needing these variables. A contrib preprocess function can always call field_info_instance() and/or field_info_field(), but do we want these variables exposed to contrib theme developers who know how to override a tpl file, but don't have the PHP skills to write a preprocess function?

The gain is small on a page where only 10 fields are themed, and that's our largest use-case in HEAD right now, but I'm concerned about #538164: Comment body as field. I don't know why it's taking so long for that to land: is Dries waiting on theme('field') optimization so the hack that bypasses it is removed from the patch? And there might not be a magic bullet: it might come down to several small tweaks like this one.

scor’s picture

@effulgentsia hop in #538164-144: Comment body as field, we're getting feedback from Dries.

effulgentsia’s picture

Title: Optimize theme('field') » [META] theme('field') is too slow, which is inhibiting its more widespread use
Category: task » bug
Status: Needs review » Active

Changing this to a meta-issue to track separate atomic optimizations that all help speed up theme('field'). Posted the #8 patch in #667040: Optimize template_preprocess_field(). The current set of issues:

#667040: Optimize template_preprocess_field()
#660856: Optimize template_preprocess()
#667038: Optimize template_process()
#667030: Optimize drupal_discover_template()

Also, reclassifying this as a bug, because the slowness of theme('field') creates a barrier to converting things that should be fields into fields. #538164: Comment body as field being the current driving use-case, but we should also consider the impact to contrib.

effulgentsia’s picture

Issue tags: +Performance

oh, and i guess it should be tagged :)

catch’s picture

Priority: Normal » Critical

Subscribe and bumping to critical. This looks to be responsible for nearly all of the slowdown between a custom field and a field API field given the results of node body and comment body.

scor’s picture

#538164: Comment body as field was finally committed which increase the importance of solving the performance issue of theme('field'). At the moment, theme('field') is disabled on comment body for performance reasons, and that means we lack RDFa support for that field. I created #664602: Wrap comment body in a tag a few weeks back which explain this particular case but hopefully it'll become duplicate of this meta issue.

There hasn't been any update here for a week or so, what issue/patch should we look at to enable theme('field') on comment body with optimized performance please? How about having a theme function (as opposed to a template file) for comment body while keeping the template file by default for other fields? Themers could still switch comment_body to a template if they want to as long as they're aware of the performance consequences.

catch’s picture

These two are both RTBC and ought to help at least a bit.
#660856: Optimize template_preprocess() #667038: Optimize template_process()

Would also be worth testing with and without #667030: Optimize drupal_discover_template().

yched’s picture

Reading a bit through meatsack's XHProf profile data at http://rugbybreakdown.net/xhprof/xhprof_html/?run=4b463a562db67&source=d...
(see #615822-82: Benchmarking / profiling of D7)

drupal_render@4 : thats the rendering of the 10 '#theme' => 'node'.
42% of a /node request.

run_init::garland/node.tpl.php: execution of node.tpl.php - almost 100% of the time is spent in the 5 calls to render() per node, 1 of them is "render( body )"
10% of a /node request

- 10% to render 10 bodies is a lot, but
- not sure where the rest of the time is spent between drupal_render('#theme' => 'node') and 'execute node.tpl.php'

Also, drupal_discover_template() is accounted for 1,4% of the request. Optimizing it might be worth it, but no miracle to expect on that side :-(

effulgentsia’s picture

Title: [META] theme('field') is too slow, which is inhibiting its more widespread use » [meta issue] theme('field') is too slow, which is inhibiting its more widespread use

Even better than optimizing drupal_discover_template() is getting rid of it and letting the theme registry work for us: #678714: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance

yched’s picture

meatsack put a modified version of node.tpl.php on his site that let us differentiate the various render() calls in there:
http://rugbybreakdown.net/xhprof/xhprof_html/?run=4b4706202fb3d&source=d...

render('node body') is 7% of a /node request
render('node links') is 2% of a /node request

effulgentsia’s picture

For anyone following here, #667040: Optimize template_preprocess_field() and #667038: Optimize template_process() landed: yay! I posted benchmarks on #660856: Optimize template_preprocess(), and that one's RTBC. I also posted benchmarks on #678714: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance, but that patch needs review.

Once those remaining 2 issues land, the cost of letting comment body be themed with theme('field') should be at around 4% on a node with 50 comments page. Is that acceptable or do we need more optimization before we can use theme('field') for comment body?

webchick’s picture

I'm tagging this issue as something to focus on getting in before alpha. According to effulgentsia on IRC, with comment module currently bypassing theme('field'), not only is this a wtf for themers, but we have to then figure out an alternate way to get RDF markup in there.

Therefore, fixing this before we release the first version of Drupal 7 that will be tested on a wide-scale would be extremely prudent.

plach’s picture

subscribe

catch’s picture

I think we can mark this fixed now that #678714: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance is in, but I'll give effulgentsia the last word before marking as such. Nice work!

webchick’s picture

Something I don't think we've done yet though is remove the hack from comment.module that prevents comment body from using field.tpl.php, due to performance concerns.

scor’s picture

yched’s picture

yched’s picture

yched’s picture

And probably very minor in #684004: Minor optimisation in template_process_field(), but brings consistency with what went in with #667038: Optimize template_process()

effulgentsia’s picture

#27-#29 are nice.

Potentially, additional minor improvements will be found too, but minor improvements can always be found, so no point keeping this issue open just for that once 27-29 are in.

However, while we did get significant improvements into theme('field'), this issue failed in its original goal of getting the improvements to the point where theme('field') is no longer a performance barrier to making something a field. According to Moshe in #652246-15: Optimize theme('field') and use it for comment body, the threshhold is at 1-2%, and I don't see us accomplishing that with theme('field').

So, I submitted an alternate attempt in #652246-25: Optimize theme('field') and use it for comment body: a brand new theme function theme_field_lightweight(). If that's a community-acceptable solution and lands, then I think the spirit of this meta issue will have been achieved, and can be closed.

effulgentsia’s picture

I just re-read #22, and want to share my thoughts about that in relation to the extra display setting option of 'lightweight' introduced in #652246-25: Optimize theme('field') and use it for comment body. Obviously, that's not completely free of WTF. But, given the options I've thought about, I think it's the least WTF. Making comment body a completely unique thing is a WTF. Expanding the "field" concept to include "normal fields" and "lightweight fields", and letting themers know that markup added to field.tpl.php (or a suggestion like field--FIELDNAME.tpl.php) is applied to normal fields, but for performance, some fields are lightweight, and if you need to customize the markup of those, you need to add a THEMENAME_field_lightweight() function (or a suggestion like THEMENAME_field_lightweight__FIELDNAME()). Similarly there's hook_preprocess_field() for normal fields, and hook_preprocess_field_lightweight() for lightweight fields. While more for a themer to know than if all fields were "normal", it's the same number of things for a themer to know vs. special-casing comment body, but once they know it, the knowledge is reusable to other high-performance-needs fields.

effulgentsia’s picture

Status: Active » Closed (fixed)

Closing this issue, because the early spin-off issues it was tracking have landed, conversation on the remaining big one should take place here: #652246: Optimize theme('field') and use it for comment body, the remaining little ones and future ones that come up can be tracked with the "Performance" tag, and it's just oh so satisfying to see the number of critical bugs outstanding decrement.