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.
Comment | File | Size | Author |
---|---|---|---|
#14 | drupal_unify_theme_suggestions-678714-14.patch | 30.51 KB | effulgentsia |
#9 | drupal_unify_theme_suggestions-678714-9.patch | 27.99 KB | effulgentsia |
#5 | ab-678714.txt | 4.73 KB | effulgentsia |
#4 | drupal_unify_theme_suggestions-678714-4.patch | 28.15 KB | effulgentsia |
drupal_unify_theme_suggestions.patch | 27.33 KB | effulgentsia | |
Comments
Comment #2
catchNo time to review at the moment but subscribing.
Comment #3
yched CreditAttribution: yched commentedsubscribe
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedCNR for bot.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedThe 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.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedI 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 proposearray('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.Comment #7
catchtagging.
Comment #8
catchOK 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.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedJust a re-roll and bot check.
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedNode 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.
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedAdding 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:
Comment #12
Dries CreditAttribution: Dries commentedThis can still go in for me. The gain can be significant.
Comment #13
webchickRe: #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.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedHere'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.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedHere'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):
So, we discussed these concerns and here's what we came up with (these are ordered to match above):
$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.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 wordtemplate_file(s)
anyway and just not have it be fully accurate, but he disliked that idea and prefers accuracy.Finally, I'm re-tagging with "API change", because I think #12 was a cross post, and not an intentional removal of the tag.
Comment #16
yched CreditAttribution: yched commented'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.
Comment #17
effulgentsia CreditAttribution: effulgentsia commented'suggestions' seems risky to me. anyone else want to chime in about '_suggestions'?
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedthere's also the possibility of the reverse: just 'theme_hook(s)'. the old variable is 'template_file(s)', not 'template_file_suggestion(s)'.
Comment #19
jensimmons CreditAttribution: jensimmons commentednode--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.
Comment #20
webchickI 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.
Comment #21
webchickActually, let's split the difference. "template_file_suggestion(s)" (thanks, Jacine!)
Comment #22
moshe weitzman CreditAttribution: moshe weitzman commentedThats 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.
Comment #23
webchickHrm. :\ 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.
Comment #24
JacineHey 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. :)
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedWow! 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.
Comment #26
webchickMy bad. :( Committed those changes.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedhttp://drupal.org/update/modules/6/7#theme_hook_suggestions_2
http://drupal.org/update/theme/6/7#template_files_double_hyphen
Comment #29
Alan D. CreditAttribution: Alan D. commentedReopening 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
Comment #30
jhodgdonFixing tag since it's specifically to be documented in the update guides
Comment #31
jhodgdonMost 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.