Optimize theme('field') and use it for comment body

#538164: Comment body as field was committed http://drupal.org/cvs?commit=310950 so setting this back to review

Ahhhh. Much better.
But can we get some before/after benchmarks?

effulgentsia posted benchmarks elsewhere which indicated a 4% hit on 50 comments once all the patches are in. Wouldn't hurt to do dedicated benches for this actual patch though, subscribing for later.

i'm preparing benchmarks for this now... will post soon.

This is a re-roll of #6 plus the addition of a couple small optimizations to template_preprocess(). Benchmarks are for anonymous user viewing a node with 50 comments displayed out of 93 comments total (I asked devel_generate to make 300, but it picks a random number between 1 and the number you ask for).

Bechmarks with #6 were disappointing:
HEAD: 205.7ms
Patch 6: 220.0ms (+7.0%)

So, I tried to investigate, and found that template_preprocess() had some changes made to it from when I first opened #660856: Optimize template_preprocess(). An optimization I had made to avoid calling path_to_theme() had gotten lost in a re-roll. And, an extra slow call to drupal_html_class() had been added in some other issue. So, I tried optimizing these two things, and got:

HEAD + #10's theme.inc hunk only: 202.8ms
HEAD + full #10 patch: 215.2ms (+6.1%)

One could stretch and call #10 as only costing 4.6% (215.2ms vs. 205.7ms), but that's doesn't reflect the real cost of theme('field'), since #10's theme.inc hunk can be applied separately.

First off, 6% is much better than the 10% I was seeing back on #538164-126: Comment body as field. But it's worse than the 4% I was estimating recently. Why? Sorry, but simply poor math on my part when I was estimating. The optimizations in #660856: Optimize template_preprocess() and #667038: Optimize template_process() optimized more than just theme('field'), so some of their benefit already made HEAD faster, and that leaves a reduced benefit when measuring the cost of theme('field') alone.

So, can this patch go in? Tough call. Some people won't like the cost. But, it's needed for RDF (otherwise, another solution for RDF will be needed), and it avoids a WTF for why comment body is different than other fields. More optimization may still be possible, but not before alpha. My vote would be to let it go in. Someone who really needs every drop of performance and doesn't need RDF or the other benefits that come from comment body themed as a full-fledged field can disable the rdf module and implement a custom module with a hook_comment_view_alter() implementation that unsets #theme. Ok, yeah, maybe I'm just looking for an excuse to make the 6% not seem as bad as it is.

scor: benchmark on a system running APC or another opcode cache. On a system without opcode cache, all templates are slow, and this can still be optimized if we care about performance on non-APC installs. Separate issue.

While a different issue, this patch may help to offset some of the cost, at least one sites with RDF enabled #683590: user_load still being called for every node view.

I'm not available the rest of the day, but one final thought I'll leave for the day is that with the way HEAD is now, if #10 lands, and someone doesn't like it for their high-performance-needs site, in addition to having the option to implement hook_comment_view_alter() to unset #theme to recover the full performance cost, they can also implement the following code in their theme:

function THEMENAME_field__comment_body($variables) {
  return render($variables['items']);
}

And this recovers half the cost, but leaves the developer free to add to this function (i.e., include attributes if rdf is enabled, include the label if the site can have a label for the comment body, etc.).

One can also implement a simpler field--comment-body.tpl.php file, though this doesn't recover as much (templates are still slower than functions, though not by as much as they used to be).

IMO, performance loss has to get down to the 1-2% range to be acceptable here. At this point, I suggest implementing an alternate solution for rdfa.

I created 3 patches that should let us squeeze a couple additional ms.
#683874: optimize template_preprocess_field()
#680022: template_preprocess() is too slow in generating valid CSS class name - followup for a slowdown that went in - my bad.
#684004: Minor optimisation in template_process_field()

Probably nothing spectacular though :-/

Spent some time digging in the profile logs generated for http://rugbybreakdown.net/ over at http://rugbybreakdown.net/xhprof/xhprof_html/?run=4b4e34ee2a91c&source=d... (that's for a front page, thus nodes only, no comments)

All in all, and unless I'm mistaken, the overhead of $element['#theme'] = 'field' vs unset($element['#theme']) (which comment.module currently does for its body) entirely lies in those functions:

- theme_render_template() - AFAICT, nothing much to flesh out around the call to drupal_render($element[0]) that current HEAD also does (only, outside of field.tpl.php, rendering the actual value without the field wrapping)
- template_preprocess()
- template_preprocess_field()
- template_process()
- template_process_field()
- rdf_preprocess_field() - but that one is precisely the feature we're looking to add back in. And I think/hope the benches were run with rdf disabled ?
[edit: I forgot the call to theme() itself]

So apart from squeezing every bit of the 4 functions in the middle, I'm a bit at loss.

I have another idea to take advantage of the way theme hook suggestions now work which can combine the performance of #22 while retaining better consistency with the field system. But it requires #241570: Theme preprocess functions do not get retained when using patterns. Any chance of having that one land? It's a forward port to D7 of a critical D6 bug fix that landed over a year ago.

I found a way to do what I was thinking of without needing #241570: Theme preprocess functions do not get retained when using patterns, though that issue should still be committed on its own merits.

Here it is. It introduces a new field instance display setting, 'lightweight', and a matching theme function theme_field_lightweight() for theming a really simple field (no label, no markup between individual items if the field is multi-valued, no default CSS classes). The purpose is to remove the performance barrier from a decision to convert something to a field, if it should be a field. Even if comment body is our only use-case in core of something that's a field and needs to be high-performance, let's solve it in a way that enables other high-performance fields in contrib, rather than special-casing a solution for comment body only. Setting a field to display as lightweight results in identical markup as if it weren't a field, with minimal performance cost, but with the flexibility to extend and be tied into the field system.

This patch does not include UI for an administrator to decide which fields to theme as lightweight vs. not. My thought is that such a UI belongs on the "manage display" screen (same place you decide on what formatter to use and what to do with the label), but should be dealt with in a separate patch. This patch simply defaults that setting within comment.module for the comment_body field. This also means that if you want to test this patch, you need to do a fresh Drupal install after applying it in order for the 'lightweight' setting for comment_body to "take". Or, you can manually mess with the 'field_config_instance' table.

By implementing a separate theme function for this, we can have a really fast default implementation, implemented as a function (faster than a template), and with a minimal set of preprocess/process functions.

Benchmarks with standard profile (rdf module enabled):
HEAD: 199.0ms
Patch: 203.4ms (+2.2%)

Benchmarks with standard profile, but rdf module disabled:
HEAD: 171.9ms
Patch: 174.2ms (+1.3%)

So, yeah, there's still a cost to themeing a lightweight field, even with rdf disabled, compared to not invoking a theme function at all, but it's pretty minimal. The rdf module is costly on its own, and this patch makes it even costlier, but that's because extra RDF markup is added. It's similar to #19. I don't think we can get extra RDF markup in the comments without paying for *some* clock cycles.

Grrr. Had to update a test. It's commented in the patch.

+++ modules/field/field.module	14 Jan 2010 23:22:27 -0000
@@ -804,6 +807,62 @@ function template_process_field(&$variab
+ * displayed, child items are simply concatenated with no extra makup, and a
+ * single wrapping DIV tag is added if and only if a preprocess function added

Typo for 'makup'.

Apart from that typo, this seems RTBC to me - please mark as such after re-rolling. It's something like a 5% performance improvement on any page displaying 50 fields given the benchmarks elsewhere (very easy to do apart from the comment module use case), and I agree it's a better trade-off than completely special casing the comment field - which means either no other fields benefit from it, or we'll get more weird special casing from people using core's example.

Also agree that a UI should be left to another issue - although it'd probably be good to put one in on balance - since people might still get confused why their labels don't show up etc.

Powered by Dreditor.

How does the 'lightweight' display as implemented in #25 behave if the field is multivalued ?

The RDF numbers in #24 look really scary, is that from before or after #683590: user_load still being called for every node view went in?

More accurate title. Bumping to critical because being able to optimize every field like this if you need to could be potentially a big win overall.

Fixed the typo found in #26, and setting to RTBC as per that comment.

Re: #27: Setting a field display to 'lightweight' results in theme('field_lightweight') being called instead of theme('field'). The default implementation of theme_field_lightweight() calls drupal_render_children($element), so all items in a multi-valued field get rendered, just concatenated together with no special markup around each item. This can be overriden by the theme globally (i.e., THEMENAME_field_lightweight()) or for a more targeted field (i.e., THEMENAME_field_lightweight__FIELDNAME()).

As a summary, what makes theme_field_lightweight() fast compared to theme('field') is no label handling, no default CSS classes, no markup around each item other than what the items itself renders, and none of the generic preprocessing. I like all the generic preprocessing and markup that theme('field') does, but the lightweight option is for when it's not needed and the overhead isn't wanted. While more could be added to theme_field_lightweight() (such as label display or some default CSS classes), we'd quickly run into the performance barrier we're trying to solve. As implemented, we keep it minimalist, and let modules/themes add preprocess steps or overrides as needed, within the performance constraints they're trying to work within.

Re: #28: The numbers in #24 were generated very shortly before that comment was posted, so after #683590: user_load still being called for every node view. Yes, rdf.module can use some serious optimization across the board. Some of the overhead is processing related stuff, and some is due to extra markup being generated, and therefore larger strings being passed around as the page is being rendered. I have not delved into profiling where the trouble spots are, but clearly it would suck if many people start disabling it on their production sites, since the goal of the module is to help create a better semantic web, so optimizing the module will be very valuable.

I cannot say I'm thrilled by the current theme_field_lightweight solution :-/.

In #23, effulgentsia mentioned a possible gain depending on #241570: Theme preprocess functions do not get retained when using patterns. What can we (roughly) expect there ?

Other than that, if we bite the bullet and accept the principle of moving the theming of "some fields" to a theme func rather than a template,

- do we really think supporting the 'label: inline or above' logic and wrapping of multiple values in separate divs would significantly increase execution cost of that theme func ?
(IIRC, #659788: [meta issue] theme('field') is too slow, which is inhibiting its more widespread use started off as a patch mirroring the current field.tpl.php as a theme func)

- should we reverse the logic and make field.tpl.php opt-in rather than opt out ? Customizing the field HTML is way simpler on a template, but it remains by far less frequent than fields displayed with the default markup.

PS: Sorry for the skeptical tone. Goes without saying: *many* thanks effulgentsia for your work and efforts in this area.

Hmm, those are all good points, including switching the default. Alpha 1 is out, so we've got time to discuss this more, back to CNR.

One thought in case we do keep lightweight ...

It doesn't look like there is anything field specific about theme_field_lightweight. Rename to theme_lightweight? Seems generally useful for other elements to output via #theme = 'lightweight'. Also, that drupal_render_children looks suspicious. If you implement this as a #theme_wrappers, you will received fully rendered content in $element['#children']. Been a while since my head was in this code.

Alpha 1 is out, so we've got time to discuss this more, back to CNR.

Yeah, sorry, webchick, that we couldn't quite get something up to par in time, but I'm glad we now have the time to work the kinks out.

In #23, effulgentsia mentioned a possible gain depending on #241570: Theme preprocess functions do not get retained when using patterns. What can we (roughly) expect there ?

No, my thought when I wrote that was to essentially do the same thing as the lightweight approach, but using suggestions passed to theme() to switch based on lightweight or not. I actually prefer what this patch is instead. The way I was first thinking of wouldn't be any faster, and IMO, would be less elegant than this.

Other than that, if we bite the bullet and accept the principle of moving the theming of "some fields" to a theme func rather than a template,

The reason I'm now leaning towards biting this bullet (compared to weeks ago when I still retained hope of getting theme('field') sufficiently fast) isn't just that theme functions are still faster than templates (though the difference has gotten smaller), but that there's other differences between targeting performance and targeting themer-friendly genericism and module-friendly extensibility. For example, the default CSS classes (field field-name-NAME field-type-TYPE), the zebra striping of field items, and the extra div around the field items to target them as a set separately from the label are all extremely useful things to have available out of the box. But they all take clock cycles. It seems to me like in many cases we want the genericism and don't have so many fields on the page that the extra clock cycles are a problem. But, in some cases, we need performance and can live without all the extra fluff (especially if the alternative is for it to not be a field at all, in which case we wouldn't get all that fluff anyway). While we could try for some middle ground (like a theme function that outputs the fluff faster than the current template does), I think it would be the worst of both worlds rather than the best -- it would still be slow enough that performance freaks would resist making something a field (if the decision threshhold is in the 1-2% range as Moshe suggests), and also be less accessible to easy theming for novice developers who are only working on simple sites where that level of performance isn't as critical. And it's not just a hybrid theme function that would need to worry about both maximum performance and maximum utility, but every preprocess function would have that same conflicting problem to resolve. By splitting the use-cases into two theme hooks, each hook can be well targeted to what's more important -- 'field' for most utility and easy theming (so, template and preprocess functions that expose lots of useful stuff) and 'field_lightweight' for performance (so, function and minimal preprocessing).

do we really think supporting the 'label: inline or above' logic and wrapping of multiple values in separate divs would significantly increase execution cost of that theme func ?

Label: no, because we can be conditional on whether the label is hidden, so for hidden label, virtually no cost to add an if statement, and for displayed label, well, the administrator chose to display it, so the time it takes to do so isn't so bad. This patch fixes that. Separate divs for field items is more problematic. Yes, there is a cost. And what is gained? Zebra-striping? If you want that, don't set the display to lightweight.

should we reverse the logic and make field.tpl.php opt-in rather than opt out?

Assuming it's not simply a template v. function decision, but a lightweight v. not-lightweight decision (including useful extra markup v minimal markup and lots of preprocessing v minimal preprocessing) as I argue for above, then no, I think the default should be what's of greatest utility rather than maximum performance. I suspect most people will use fields as stuff you add to a node for viewing that one node, and that if there are many fields on a node type, then only a few are set to display on the teaser, so the performance difference isn't all that big. Better for novice users to have that as the default, and for advanced users (module developers building special fields like comment body and site builders working on complex sites that have many fields per page) to apply performance where it's needed, with the understanding of what they give up (zebra striping, extra divs and css classes for easy targeting, other things modules and the theme may be doing to make non-lightweight fields look cool).

It doesn't look like there is anything field specific about theme_field_lightweight.

There is in this patch.

Also, that drupal_render_children looks suspicious.

Thanks for pointing that out. This patch removes it, shaving a few more clock cycles in the process.

I'm gonna work on this some more. Maybe there is a way to accomplish a better switching between lightweight and not and less difference between the two. Feel free to review in the meantime, or wait until I post the next one.

Separate divs for field items is more problematic. Yes, there is a cost. And what is gained? Zebra-striping?

No, the mere fact that multiple values won't be garbled together: value_1value_2value_3 :-)
Right now, field_lightweight is useless/broken on multiple values.

Maybe there is a way to accomplish a better switching between lightweight and not and less difference between the two.

Would be great :-). Right now I'm a little worried to see the two flavours fork in so very separate directions. Maintenance headache.

Ok, here's one I think you'll like, yched. I'm getting some confusing benchmark results, so I'll post those once I make sense of them. In the meantime, here's the patch.

This removes the whole 'lightweight' concept. Instead, it implements theme_field() as a function with the same markup as field.tpl.php. But, field.tpl.php is still in the folder, and the way the preprocess function is written, if someone copies it to their theme folder and modifies it, then all fields get themed using it (which would include comment body). But, instead of having a field.tpl.php in the theme folder, one can have a field--X.tpl.php, and that will only be used for that field, and the faster function will be used for all other fields. Or, one can have field.tpl.php in their theme, slowing down all fields, but have a THEMENAME_field__X() function to make a particular field faster.

This patch includes some hunks from other issues, including optimizations and #241570: Theme preprocess functions do not get retained when using patterns. Anyway, I'll post benchmarks once I can make sense of them.

Haven't reviewed the code yet, but that sounds really nifty. Looks like I need to cvs up devel to get comments being created properly.

I'll submit another patch within the next day or two. Might as well hold off review until then, as it will have some new things in it.

.

Yay ! If the perf results live up to the expectations, I think the latest approach is just brilliant. Best of both worlds :-).

I'll wait for the next patch to post a more specific review.

Thanks, scor. I'm curious how this one does performance-wise. CNR for bot. Will post benchmarks a bit later.

One thing that's difficult is that simply benchmarking relative to HEAD isn't the best comparison, because the patch includes some optimizations which can be applied separately, independent of making comment body render as field. I will try to supply information for HEAD, just the optimizations that can be applied separately, the effect of making comment body use the more optimized theme('field') but without using the new #lean_markup feature in this patch, and the effect of making comment body use the more optimized theme('field') combined with the #lean_markup feature. Then we can make decisions as to whether the different optimizations are worth the code complexity.

This patch includes #lean_markup as a replacement for what used to be 'lightweight' in earlier patches. We now have two distinct optimizations: function implementation over template implementation, and outputting less markup compared to more markup. Each has an effect and this lets us evaluate and combine as needed. Unlike previous patches, this one takes an approach that 'lean_markup' is not a a setting for the administrator, so no UI is needed. It is strictly for modules and themes to control within hook_field_attach_view_alter().

A code review at this time is welcome, but if you want to wait for benchmarks, that's cool too.

So. Very. Frustrating. Here's a patch without the lean_markup related cruft and is nice and simple.

I'm also adding a text file that's a patch with my latest work on the lean_markup idea, but I think it's a dead-end, at least for core (though it may make a nice contrib module or a tip for people building custom themes for complex sites). When having lots of fields on a page, all the extra CSS classes and DIVs added by theme('field') does bloat the page, but I'm not getting consistent enough benchmark results to have a clear sense of how that affects page rendering performance. It improves bandwidth, but that's a separate issue, and if there's page compression on the server, the bandwidth improvement is less dramatic. Anyway, I like the idea for a contrib module or custom theme, but I'm having a hard time getting data that justifies the added code complexity in core.

So, back to just the simple #46 patch. My numbers are slightly different than scor's, but not too much. And the difference between environments (e.g., Linux v. Mac) can definitely account for the discrepency. Numbers from additional systems may help paint a better picture if the decision rests on a certain level of precision.

With RDF disabled:
HEAD: 168.0ms
Patch #46: 168.6ms
Patch #46 but re-introduce theme('field') bypass: 163.9ms

The last of the above tests is simply applying the patch, but then adding unset($comment->content['comment_body']['#theme']); within comment_build_content() after the call to field_attach_view().

So, basically, the patch includes some obvious optimization to template_preprocess() that can be applied separately to making comment body themed as a field. And the optimization to theme('field') is benefiting the node that's displayed above the 50 comments. But there still remains a 2.8% difference between the 2nd and 3rd numbers (the cost of letting comment body theme as a field) which is above the 2% threshhold suggested in #15.

This one might be within the 2% needed. I'm gonna reboot my computer to get benchmarks in a clean state.

My system is generating numbers that are too chaotic. Can someone else please benchmark #46 and #47 with RDF disabled, viewing a node with 50 comments with the 3 cases mentioned in #46 (HEAD, patch, and patch with re-introduction of theme('field') bypass). #46 describes how to do that last test, and #47 includes a hunk with an @todo above it that can be uncommented. Thanks! #47 should be faster than #46, but perhaps not by enough to make its additional optimizations worthwhile.

@scor: thank you so much for the benchmarks. Any chance you can post the same tests for #46 with lean markup (the one with the .txt extension so bot ignores it)?

Why are you comparing HEAD and "#47 with re-introduction of theme('field') bypass"?

The various patches: #47, #46, and earlier, contain optimizations that can be applied separately from making comment body use theme('field'). They're needed to make theme('field') faster, but in the process, they make other things faster too. I'm not sure that we can use those savings to justify paying an overhead on comment body. Put another way, if we were to roll a patch that contained #47 with the bypass, and had that land in HEAD, then according to #15, we would need the removal of the bypass to cost no more than 2%.

Moshe, yched, catch: please chime in here with thoughts on how to proceed. Do we need to get the other optimizations committed first, and then evaluate the comment body dilemma separately from generic optimizations?

D'oh! The problem was that I was going down too many other tracks with earlier patches. Here's a much simplified version that only optimizes theme('field') and nothing else. So, this can be compared with HEAD. On my system:

HEAD: 165.5ms
Patch: 169.2ms (+2.2%)

@scor: can you please post your numbers, that we can compare with #49 to make sure nothing funny is going on? But if this is correct, we're within half a millisecond of acceptability!

Haven't looked at latest patches but 2.2% seems more than close enough for me. The fact that core becomes overall a bit faster is a good thing, and shouldn't be a reason to keep specialising comment body - we're trading a one-off specific hack for something which benefits every rendered field.

The only reason to split patches here is if it's unreviewable in one go.

One thing I'd like to see if possible is benchmarks on 300 displayed comments (i.e. the worst case) before and after the patch - just to get an idea how flat the 2% is.

@catch: yay, thanks, glad you're on board! I think #51 is an appropriate amount of stuff to review for this issue. It's a generic speed-up of theme('field') and a removal of all comment body special casing. When I get a chance, I will submit separate issues (some already exist as separate issues) for the stuff I took out of #47, which are generic theme system clean ups and optimizations that can be reviewed separately.

@scor: please provide benchmarks for #51 for 50 comments and 300 comments from your system. I will do the same from mine tomorrow.

Impressive work.

I agree with the removal of 'lean_markup'. Different issue, different debate, I'm not sure we should start that here and now :-)
As I said above, the 'theme func by default / overridable with a tempalate' is IMO brilliant.

+++ modules/field/field.module	20 Jan 2010 01:16:09 -0000
@@ -741,69 +736,134 @@ function field_extract_bundle($obj_type,
-  $items = array_intersect_key($element, array_flip(element_children($element)));
(...)
+  // so an expensive call to element_children() can be avoided. If the keys of
+  // the item render arrays are different than the keys of #items, then the code
+  // responsible for making them different (for example, a complex
+  // hook_field_formatter_view() implementation) must set #use_element_children
+  // to ensure that the correct items are rendered.
+  $variables['items'] = array();
+  $deltas = empty($element['#use_element_children']) ? array_keys($element['#items']) : element_children($element);
+  foreach ($deltas as $delta) {
+    if (!empty($element[$delta])) {
+      $variables['items'][$delta] = $element[$delta];
+      $variables['item_attributes_array'][$delta] = array();
+    }

I'm not a fan of #use_element_children. Convoluted, and not really necessary IMO.
By contract, hook_field_formatter_view is supposed to return a render array with numeric keys starting at 0. It should be reasonable to assume that it is a subset of the values deltas (array_keys($element['#items']).
For most formatters, 1 delta -> 1 element in the render array.
For some formatters (what we previously called 'multiple formatters'), n delats -> 1 single element in the render array, keyed by 0 - e.g. the 'file_table' formatter in file_field_formatter_view().

So it seems we could loop on :

  foreach (array_keys($element['#items']) as $delta) {
    if (!empty($element[$delta])) {
      $variables['items'][$delta] = $element[$delta];
      $variables['item_attributes_array'][$delta] = array();
    }
  }

Or - In #683874: optimize template_preprocess_field(), I used

+  $items = array();
+  foreach ($element as $key => $item) {
+    if (is_numeric($key)) {
+      $items[$key] = $item;
+    }
+  }

to replace the element_children() call.

+++ modules/field/theme/field.tpl.php	20 Jan 2010 01:16:09 -0000
@@ -3,7 +3,16 @@
+ * This default template is not used, because an equivalent implementation
+ * exists in the theme_field() function. Function implementations are faster
+ * than template implementations, and because on some sites, there can be many
+ * fields displayed on a page, the faster function implementation is used by
+ * default. People building sites without too many fields per page and who
+ * find it easier to customize templates than override functions can copy this
+ * file to the theme's folder and customize it, and the customized template will
+ * be used instead of the default function.

Excellent, but this should stand out more IMO. Like, putting a big red (er...) comment note outside the usual 'Available variables' section of the tpl.php header, that many people will skip, and more in-your-face in the regular HTML part.

Could be shorter and more to the point, and direct to @see theme_field() for more details.

Powered by Dreditor.

Also : I kind of assumed the theme.inc hunks in versions earlier than #51 were required to make the 'default is theme func / overridable by template' mechanism work. Those were "just" optimizations ?

Just a quick question by someone not so into the whole theme/template system (please don't kill me!):
What's the disadvantage of making this "theme function by default - override as template file" the default for all current template files? I imagine that would result in a remarkable speed-up.

@tstoeckler: yes that sounds ideal for performance but it'd mean duplicating every template as a theme function, and making this pattern easy enough to apply everywhere, I very much doubt we'll have time to do that for D7, also the gains here are generally going to be from theme functions called a lot of times each page - so not page.tpl.php and similar.

On the numbers, 3.5% for 300 comments is 0.01% for each comment and only 1.5% more relative overhead than with 50 comments, that seems pretty good to me.

same question as yched in #55. the patch in #51 is not enough for easy copying of field.tpl.php to theme dir. would require registry_alter or some other mechanism to get drupal to use it. we should at least document how thats done.

With all feedback from above.

I added back in hunks to remove 'theme paths' from the theme registry. This has no impact on performance (just affects registry build), is a clean up that should have been included in #678714: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance, and is needed to avoid a PHP warning when a template overrides a function implementation.

I also added an extra hunk within theme() to ensure that template_preprocess() gets called for all template implementations, even ones that override function implementations. The reason for it is commented, it enabled me to remove an @todo from #51, and also results in a net 0.1-0.2% performance increase relative to #51. The performance cost of the extra hunk is an isset() call that only runs for template implementations, and can't be detected within any statistical noise.

Regarding #55/#60: The ability to mix-and-match templates/functions is straightforward and requires nothing other than the existence of the templates or functions within the theme. No registry alters. One of the nice benefits we got from #678714: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance. The above, however, clean up some minor stuff related to it. I added documentation in theme_field() so people know about this.

@yched, re: #55: some of what you may be referring to is in #241570: Theme preprocess functions do not get retained when using patterns. I pulled it out of #51/#61 because it's no longer needed for this issue. That patch is needed for when theme() is called with an array of hooks, for example, if in field_default_view(), we were setting #theme to an array of suggestions, as I was toying with in earlier patches. But that patch is not needed for when suggestions are set within a preprocess function, as we've been doing in template_preprocess_field() and can continue to do so until some other compelling reason drives us to move the suggestion evaluation to earlier in the pipeline (for example, if we want to support different preprocess functions running for different suggestions: can be addressed in a separate issue).

Beautiful. Patch #61 answers my remarks in #54, and is beautifully commented.

Last comment:

Powered by Dreditor.

+++ modules/field/field.module	20 Jan 2010 20:14:13 -0000
@@ -741,69 +736,145 @@ function field_extract_bundle($obj_type,
+  foreach ($element['#items'] as $delta => $item) {
+    if (!empty($element[$delta])) {
+      $variables['items'][$delta] = $element[$delta];
+    }
+  }
+
(...)
-  // Initialize attributes for each item.
-  $variables['item_attributes_array'] = array();
-  foreach ($variables['items'] as $delta => $item) {
-    $variables['item_attributes_array'][$delta] = array();
-  }

Don't we still need to initialize $variables['item_attributes_array'][$delta] ? (It was done in the same loop until #51, I removed it from my sample code in #54 for clarity, so I'm not sure if you left it out on purpose)

I tested the patch to work, including the easy template override. According to the benches in previous comments, I'd call this RTBC after the above is answered.
This is amazing work, effulgentsia. Thanks !

Powered by Dreditor.

Don't we still need to initialize $variables['item_attributes_array'][$delta] ?

Nope. template_preprocess() initializes the *_attributes_array() variables, and as long as it was being called in the preprocess chain, it made sense to initialize item_attributes_array to maintain consistency of expectations. With this being a function implementation now, template_preprocess() isn't called, and other preprocess functions know this (or should) and shouldn't assume that any of the *_attributes_array variables are initialized (they're not in any function implemented theme hook). Normally another preprocess function (like the ones in rdf.module) just adds to the variables, like $variables['attributes_array']['foo'] = 'bar', which PHP allows without the variable being initialized. If a hook_preprocess_field() function calls array_merge() or += or anything like that, it would need to check an isset(), but again, that's the case for all function-implemented theme hooks. Based on this, #61 re-works template_process_field() to not assume any of those variables have been initialized. Not only is this more consistent with the paradigm of other function-implemented theme hooks, it also results in faster performance when there aren't attributes, and when there are, the existence of attributes overshadows any difference between initialized and not.

Got it.

RTBC, then.

Re: #56/#58: I agree with catch. There's a couple it *might* make sense to do that for (user_picture?), and that can be up for debate in a separate issue, but generally, a theme hook falls nicely into the category of "really useful for themers scared of php code to customize and not called very many times per page" or "performance critical and less likely to need customization in simple themes". Fields are one of the ones that span both categories. There might be others, but given the duplication involved, I hope we don't go crazy with it.

Can we see some updated benchmarks here?

50 comments, anonymous user with permission to view comments, no page caching.

HEAD:

Concurrency Level:      1
Time taken for tests:   201.245 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      142699000 bytes
HTML transferred:       142173000 bytes
Requests per second:    4.97 [#/sec] (mean)
Time per request:       201.245 [ms] (mean)
Time per request:       201.245 [ms] (mean, across all concurrent requests)
Transfer rate:          692.46 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   178  201  88.8    190    1845
Waiting:      169  192  86.5    181    1802
Total:        178  201  88.8    190    1845

Patch:

Concurrency Level:      1
Time taken for tests:   198.735 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      151574000 bytes
HTML transferred:       151048000 bytes
Requests per second:    5.03 [#/sec] (mean)
Time per request:       198.735 [ms] (mean)
Time per request:       198.735 [ms] (mean, across all concurrent requests)
Transfer rate:          744.82 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       1
Processing:   182  199  17.6    194     440
Waiting:      174  190  16.3    185     364
Total:        182  199  17.6    194     440

@catch: your first number seems suspicious. It shouldn't be slower than with the patch and the stddev is too high. Probably APC wasn't well primed, or your CPU was busy doing something else during that run.

[Edit: the median numbers seem about right though, showing the patch is approx. 2% slower, which is consistent with scor's numbers, though not precise enough to know whether it's just under 2% or just over.]

Clearing cache is enough. I usually do an "ab" run of 100 after clearing cache, followed by a run of 500 or 1000, so that the second run is with well primed caches and therefore, lower stddev.

Ignore mine from #70, this needs a reinstall. Benchmarks in #69 and #57 are equivalent though so this should be good to go from that standpoint.

I get similar, but slightly different numbers than scor (this is consistent with past issues, and i think is due to us running different OS):

Node with 50 comments displayed:
HEAD: 170.1ms
Patch: 174.2ms (+2.4%)

Node with 300 comments displayed:
HEAD: 803.4ms
Patch: 829.1ms (+3.2%)

Additionally, since this patch optimizes theme('field') across the board (not just for comments), I decided to benchmark the front page with 10 article teasers, 3 fields per article (instead of devel_generate, I manually added 10 article nodes, in order to add image and tag in addition to body):

HEAD: 92.6ms
Patch: 87.0ms (-6.0%)

So as expected, the patch results in performance increase when comparing something that was already calling theme('field'), but performance decrease for comment body since it's changing it from not calling theme('field') to calling theme('field').

In #15, Moshe set a threshhold at 2% for what would be an acceptable cost to making comment body call theme('field') and therefore be RDF-compatible. Therefore, the difference between my number (2.4%) vs. scor's (1.7%) might be important and needing a 3rd system to supply a benchmark. In #52, catch suggested that he'd be ok with going a little over 2%.

Instead of doing THEMENAME_field__body__article() or field--body--article.tpl.php it feels more logical to lead with the content type. That is THEMENAME_field__article__body() or field--article--body.tpl.php.

re @Dries #76:
The patch just mirrors the order in current HEAD (and CCK D6). I'd say changing this belongs in a different thread.

Agreed. Will come back to this patch in a bit.

Committed to CVS HEAD. Thanks.

Thks again for the impressive job, effulgentsia.