Normally, I'd think this would be out of scope this late into the D7 cycle. But, we very recently got #653622: Make the '__' pattern for theme suggestions easier to use in, which brings the '__' suggestion convention out of obscurity and into core. Also, we recently got #538164: Comment body as field in, which raises the priority of improving performance of theme hooks implemented as templates. One thing that helps is a static variable cache in drupal_discover_template() (#667030: Optimize drupal_discover_template()), but with this issue, we can get rid of that function entirely, and have even better performance. Finally, we have (had it in D6 too, nothing new) an annoying clobbering WTF: #405496: Can we change the convention for template_files override from hyphen to something else in order to avoid clobbering? because '-' is used to delimit both multiple word hook names and suggestion suffixes.

So, on balance, I think this patch is worth it. I'll post benchmarks soon.

Just to clarify, the net effect of this patch is that instead of node-TYPE.tpl.php, you would use node--TYPE.tpl.php.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, drupal_unify_theme_suggestions.patch, failed testing.

catch’s picture

No time to review at the moment but subscribing.

yched’s picture

subscribe

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
28.15 KB

CNR for bot.

effulgentsia’s picture

FileSize
4.73 KB

The driving use-case for this issue is to enable comment body, which is now a field, to be themed with theme('field'). There's a code comment in comment_build_content() explaining why it's lame to not have comment body themed as a normal field. Also, the slowness of theme('field') will impact things in contrib that want to use fields liberally. There's a meta issue with more about this: #659788: [meta issue] theme('field') is too slow, which is inhibiting its more widespread use.

Benchmarks for this issue from my system attached. I'm running with APC enabled on a Macbook with stock hard drive, which isn't very fast -- systems with faster hard drives will likely experience different results. Results without an opcode cache will also be different, because the "include" statement in theme_render_template() is very slow without APC: I will open a separate issue for that if we care about optimizing for people without an opcode cache, but for now, with opcode cache is the important measure.

Fresh install of HEAD with standard profile, overlay module disabled, devel_generate enabled, node with 300 comments generated, anonymous user granted "view comments" permission and viewing that node with 50 comments displayed.

HEAD: 200.3ms
HEAD plus allowing comment body to use theme('field') (i.e., remove the hack in comment_build_content()): 215.6ms (+7.6%)
HEAD plus allowing comment body to use theme('field') plus patch #4: 210.7ms (+5.2%)

In other words, we shave approx. 1/3 of the overhead of theme('field') with this patch. Since this patch isn't specific to theme('field'), the improvement benefits all frequently called templates that use suggestions. The savings is because we don't have all that IO looking for suggestions in all the different directories each time the theme function is called. Instead, we leverage the capability of the theme registry to already know where suggestions are if double underscore/hyphen is used instead of single.

Combined with other patches that optimize template processing (#660856: Optimize template_preprocess() and #667038: Optimize template_process()) that I still need to post benchmarks for, we may get to the point where the performance penalty of letting comment body use theme('field') is acceptable.

moshe weitzman’s picture

I read through the code and it looks good.

I do scratch my head when I see a suggestion like 'block__' . $variables['block']->module . '__' . $variables['block']->delta;. Why do we use an arcane string delimiter when an array would do? I propose array('block', $variables['block']->module, $variables['block']->delta). I'm guessing that we are trying to be consistent with some other part of the code but surely I can find examples where we use arrays to collect a few items into an ordered list.

catch’s picture

Issue tags: +Performance

tagging.

catch’s picture

OK so it looks like this takes 5ms of a 200ms request in the 50 comments example, which is 2.5% - pretty nice but Dries is after 5%-ish for API changes before Friday.

I'd like to see what happens when rather than 50 comments, we have one node with 20 fields on it (not at all unheard of), that'd be a useful comparison to see what the impact is in some different scenarios.

Overall, while it's not a small change, it's a very simple one, so would be good to get some wider opinions.

effulgentsia’s picture

Just a re-roll and bot check.

effulgentsia’s picture

Node with 20 fields is only 2% savings. But, you want 5%? Here's how you can get 5%. Fresh install of HEAD, standard profile, disable overlay, enable devel_generate. Copy modules/field/theme/field.tpl.php to themes/garland/field.tpl.php and empty Drupal cache to refresh the theme registry. Add 4 text fields to Article type. Generate 100 article nodes. Update site configuration to display 30 teasers on the front page. Benchmark the front page. On my system:


HEAD: 226.9ms
Patch: 215.3ms (-5.1%)

Reason for copying field.tpl.php to garland is that in HEAD, drupal_discover_template() is increasingly slow the more paths it needs to search. It's not unreasonable to assume that a theme will want to override field.tpl.php (either to customize the markup for all fields, or to customize a specific field, like field-FIELDNAME.tpl.php, but in HEAD, if you want to implement a specific suggestion template, you also need the base template in the same folder). In fact, this test is without even using a sub-theme. Add a sub-theme, and the difference gets bigger, because drupal_discover_template() becomes even slower, while with this patch, performance doesn't degrade with sub-theme layers, because everything is figured out during theme registry building.

So this tests 150 calls to theme('field') (5 per node * 30 node teasers). How realistic is that? Well, the article type has "image" and "tags" fields already (not used by this test, because devel_generate doesn't populate them). Add something like "publication date" and "publication journal", and you've got 5 fields per article. And since we provide the option for someone to bump up to 30 teasers on the front page, I suspect quite a number of websites will choose it. And I know that I've built websites using views that result in way more than 150 fields on a page, and I suspect so have others.

So, the performance gain from this patch scales with number of fields. As already mentioned, it also scales with depth of sub-theme heirarchy. And, it scales with number of suggestions. template_preprocess_field() offers 3 suggestions (not counting 'field' which isn't really a suggestion). But if a contrib module adds more suggestions (let's say for the field type), with this patch, performance remains constant, while HEAD slows down.

Finally, the performance gain from this patch is in disk IO savings (removal of file_exists() calls). This means that on a production server with fast hard drives and an OS that has good file system caching, the savings would likely be much smaller than on my Macbook. However, high traffic websites are often disk IO bound, so on a server whose disk is already being hammered, the gain can be larger.

effulgentsia’s picture

Issue tags: +API change

Adding this as a separate comment to keep #10 strictly about performance. Ultimately, performance is going to be the determining factor that justifies the API change (or not) this late in the D7 cycle. But in addition to performance, as the issue title suggests, I see some other (IMO, important) benefits:

  • Consistency of suggestions. We now have in core the construct of calling theme() with explicit or implicit suggestions that use the '__' convention. For example, theme('links__node', ...), which will look for a 'links__node' implementation, otherwise default to 'links'. While this is new to core as of a couple days ago: #588148: theme_links() is not really themeable, anyone who has themed views in D6 is used to views using this pattern, and the resulting '--' delimiter for template file names. And the reason D6 views used this pattern is precisely because of the performance gain from doing so as well as the other benefits I'm about to list. And now that the pattern is used by core, I expect more D7 contrib modules to use it. Why would we want themers sometimes having template file name suggestions delimited by '-' (just because that's been the Drupal legacy) and sometimes by '--' (for modules like views that care about performance)? While D6 themers will need to adjust from '-' to '--', it's not that hard an adjustment, and the consistency gain is more than worth it, IMO.
  • The above consistency also allows for interoperability. What's the difference between specifying suggestions when calling theme() vs. specifying them in a preprocess function? Code should be able to swap between these freely, based on whether the code calling theme() knows which suggestions are appropriate, or whether it makes more sense for the preprocess function to determine the suggestions. Using a different delimiter between the two makes no sense. The concept of a suggestion is the same regardless whether the suggestions are known at invoke theme() time, or within preprocess time.
  • In HEAD, the base template has to exist in the same folder as the suggestion for the suggestion to be found. In other words, if in your theme, you want a field-FIELDNAME.tpl.php file, you also must have field.tpl.php in there, even if you have no desire to override it. This is confusing to themers and senseless. This requirement exists because drupal_discover_template() would be even slower than it already is if it had to search all possible folders every time it was invoked. With this patch, however, we leverage the fact that during theme registry building, we have the luxury of taking as long as we want, and that drupal_find_theme_templates() already is capable of finding templates anywhere, so themers would be able to have a field--FIELDNAME.tpl.php file anywhere they want in their theme folder, without also needing a field.tpl.php file in there.
  • In HEAD, we have all sorts of clobbering pitfalls. If you have a content type named 'form', and you put a 'node-form.tpl.php' file in your theme, you can no longer edit any node of any type, because the theme system treats 'node-form.tpl.php' as an override of theme_node_form() in addition to the 'node-form' suggestion of the 'node' template. It would be so nice to finally have that fixed, and let '-' separate words of a theme hook, and '--' separate suggestions.
Dries’s picture

This can still go in for me. The gain can be significant.

webchick’s picture

Issue tags: -API change

Re: #11: Wow. That first one is going to be a major pain for themers at this stage in the cycle. OUCH. :\ However, trading it off for the second one, as well as the consistency and performance benefits, is going to be a big win. I've never personally hit (or seen our students hit) that node-form.tpl.php problem before, but have definitely see them hitting #2 a lot, as well as being totally confused when we get to Views templates and there's that weird -- vs. - thing. And if Dries is okay with it... I guess okay, then.

I'm pretty ambivalent to what this looks like under the hood, but as long as we're throwing BC out the window, might as well do it in the least WTF-inducing way possible.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
30.51 KB

Here's a patch with the @todos removed: it adds 'poll-bar--block.tpl.php' and 'poll-results--block.tpl.php' which are just copies of 'poll-bar-block.tpl.php' and 'poll-results-block.tpl.php'. When this patch is committed, the latter need to be deleted (nothing will break if they're not deleted, but it'll be less confusing to not have old files left around). If there's a way to roll a patch that includes file deletion, it would make for a great handbook page :)

Setting to RTBC because in IRC, Moshe tentatively approved it (overriding his request in #6). I will follow up with a comment with a summary of the IRC chat that I, Moshe, Crell, and webchick had.

effulgentsia’s picture

Issue tags: +API change

Here's a summary of several IRC chats about this issue between me, Moshe, Crell, and webchick. First, concerns raised about this patch (not in order of priority):

  1. This patch makes it so that ALL template file names for suggestions uses the '--' delimiter instead of '-'. So, for example, a theme that implements 'node-article.tpl.php' must change it to 'node--article.tpl.php'. There's a concern that this is ugly.
  2. Related to above, preprocess functions that add suggestions will have to use the '__' delimiter when adding the suggestion. See Moshe's comment #6. There's a concern that this, too, is ugly.
  3. With this patch, the new variables set in preprocess functions are named 'theme_hook_suggestion'/'theme_hook_suggestions' (replacing the variables 'template_file'/'template_files'). There's a concern that the new variable names are ugly.
  4. Regardless of the subjective feelings about the ugliness of above items, this patch comes about 3 months too late. Code freeze was 10/15, some people have started to port modules and themes. People who've ported their modules already will need to re-update them to replace 'template_file'/'template_files' with 'theme_hook_suggestion'/'theme_hook_suggestions'. People who've ported their themes already will need to rename all template files that implement suggestions.

So, we discussed these concerns and here's what we came up with (these are ordered to match above):

  1. Views in D6 already uses the '--' convention for separating suggestions. For example, views-view--VIEW-NAME--DISPLAY-ID.tpl.php. We tried to find merlinofchaos to ask about his thoughts about changing this in the D7 version of views to views-view-VIEW-NAME-DISPLAY-ID.tpl.php. We couldn't find him, so Crell represented the views module and argued against this. With so many different kinds of "objects" in views, and each of them can be named anything, the potential for clobbering is too high (see #11.4). IMO, whatever suggestion delimiter we use in views, we should also use for fields, because fields and views are so related to each other, it would be a major WTF for why it's '-' for fields and '--' for views (that WTF exists in D6 between CCK and Views, but now that fields are in core, we should be more concerned about it). Also, if the argument is that clobbering potential is too high in views, I don't see why it's any lower in fields. If we go with this reasoning and have '--' as a delimiter for both views and fields, why not be consistent and use it everywhere?
  2. As per #6, it would be possible to make the '__' hidden from preprocess functions. For example let preprocess functions set $variables['theme_hook_suggestions'][] = array('block', $variables['block']->module, $variables['block']->delta) instead of $variables['theme_hook_suggestions'][] = 'block__' . $variables['block']->module . '__' . $variables['block']->delta. But, if template file names must use '--', is anything gained from hiding '__' from preprocess functions? If the arcane delimiter can't be fully hidden, what's useful about partially hiding it? Also, it would introduce an inconsistency between calling theme() and setting this variable (see #11.2). Moshe reluctantly agreed to this and withdrew his request.
  3. Any ideas for a better name than theme_hook_suggestion(s)? Please share them. The performance gain from this patch is due to the fact that at run-time, after theme() runs the preprocess functions and has suggestions to look for, it no longer does it be searching for template files, but by inspecting the registry for theme hook implementations (which were auto-discovered by scanning for template files during registry build time). I asked Moshe whether it makes sense to stick to the word template_file(s) anyway and just not have it be fully accurate, but he disliked that idea and prefers accuracy.
  4. The final point is the most difficult one. Yeah, it sucks. Other than webchick pointing people to this issue and shifting some of the flame that comes her way to the rest of us, I'm not sure what to do about this.

Finally, I'm re-tagging with "API change", because I think #12 was a cross post, and not an intentional removal of the tag.

yched’s picture

'theme_hook_suggestion': why not just 'suggestions' ? or '_suggestions' if we don't want to clash with a possibly valid template variable ?

I'll add that if this gets in and suggestions become cheap, I strongly vote for an additional node--[type]--[view_mode].tpl.php sugestion - which I'd probably end up adding myself in most of my themes. Followup job.

effulgentsia’s picture

'suggestions' seems risky to me. anyone else want to chime in about '_suggestions'?

effulgentsia’s picture

there's also the possibility of the reverse: just 'theme_hook(s)'. the old variable is 'template_file(s)', not 'template_file_suggestion(s)'.

jensimmons’s picture

node--article.tpl.php is ugly compared to node-article.tpl.php, and that bit of ugly is annoying.
But there's a good reason for this, so I'm sold.

We'll get used to it. And at least it's a fast and easy change in upgrading themes from D6 — and one you can spot right away.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I wanted to get feedback on people who'd be directly affected by this patch, so I talked this change over with Jacine and jensimmons in #drupal-design. Basically, they said that a template file rename wasn't a big deal; just one of 11 billion things they're going to need to do to port their themes anyway. And when we discussed the other benefits, particularly the fact that this would allow node--XXX.tpl.php to work without having a node.tpl.php in there, they were basically sold. :)

Since Dries already gave his thumbs up, I think this is good to go.

But one thing. I really don't like the change from 'template_file(s)' to 'theme_hook_suggestion(s)', and Jacine confirmed that "hook" implies an actual hook called suggestions (which was my thought as well) and so that was confusing. That change would possibly be fine if it were internal to theme.inc, but this is a themer-facing property; we often add additional suggestions in preprocess functions.

Furthermore, if we're going to mess with the name, we should think up something much better than 'singular name' / 'plural name' since we don't use that construct anywhere else in core, and we really don't have time to bikeshed that atm. :P

So get rid of that change (which will also slightly lessen the API change here), and then we're good.

webchick’s picture

Actually, let's split the difference. "template_file_suggestion(s)" (thanks, Jacine!)

moshe weitzman’s picture

Thats fine, except for the fact that we are not only talking about templates or files here. The values of this property can refer to theme functions as well.

It isn't widely used, but 'theme hook' is not a new word. its in all our d7 docs. search for it at http://api.drupal.org/api/function/theme/7 and http://api.drupal.org/api/group/themeable/7 and http://drupal.org/node/165706.

I was stuck on the word hook too. But I eventually got over it. Think of it this way: a theme hook is a chance for any theme raise its hand and take over the handling. Its just like a module hook, except the theme system picks a single theme as winner, whereas module system lets everyone play.

I don't recommend adding confusion by putting template in the name of this property.

webchick’s picture

Issue tags: +Needs documentation

Hrm. :\ Ok. :\ I still stand by the fact that I think this is introducing a serious WTF here for 99% of themers who don't know/care how this property works under the hood.

However, it's not worth holding the patch up to have this discussion, so committed to HEAD. Though I'd love to see us come up with something better before Friday a.m.

The changes introduced by this patch need to be documented in the module upgrade guide. Tagging.

Thanks, effulgentsia, for this clever performance improvement and general clean-up of the underlying theme system.

Jacine’s picture

Hey guys,

I was confused because I thought theme_hook_suggestions was a function name, but I see that it's just a variable so it's no big deal IMO. Also, I think the template file name changes are fine, and that it will be easier to explain to people at the end of the day. :)

effulgentsia’s picture

Wow! I just got home and saw this, cool! Not sure if it's just my CVS client, but it seems the rename of poll-bar-block.tpl.php and poll-results-block.tpl.php to poll-bar--block.tpl.php and poll-results--block.tpl.php didn't happen. If it's just my CVS client, don't worry about it, I'll figure it out. But if that's the case in HEAD, then it will cause test failures, and mess up other patches.

webchick’s picture

My bad. :( Committed those changes.

effulgentsia’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Alan D.’s picture

Status: Closed (fixed) » Active

Reopening as I tried this for the first time via a module and had one of the biggest WTF moments that I've had for years. I'd suggest adding this to the modules update page, but I'm not sure about the state of this after reading other related threads. If correct, it should be added to http://drupal.org/update/modules/6/7#theme_hook_suggestions_2

The theme system automatically discovers the theme's functions and templates that implement more targeted "suggestions" of generic theme hooks. But suggestions implemented by a module must be explicitly registered by that modules hook_theme() function.

jhodgdon’s picture

Fixing tag since it's specifically to be documented in the update guides

jhodgdon’s picture

Status: Active » Fixed

Most of this had already been documented in the module/theme update guides (see #27).

The only remaining item was a suggestion in #29. But the text that is suggested to be modified does not exist there any more, and also this behavior is not new in D7 (same thing in D6), so I don't think this needs a doc change.

Status: Fixed » Closed (fixed)
Issue tags: -Performance, -API change, -Needs documentation updates

Automatically closed -- issue fixed for 2 weeks with no activity.