template_preprocess() is too slow in generating valid CSS class name

This is correct, according to the new API for class names.

Hm. Is it possible to write a test for this?

Ok, fair enough.

Committed to HEAD.

Yes. This one's a no-brainer. The same hunk is in #652246-10: Optimize theme('field') and use it for comment body with a discussion about its impact. That hunk also has a more detailed code comment, if that's of interest. Makes sense for this hunk to land separately of that other issue which is still being worked on.

Sorry for downgrading an RTBC patch, but whether in core or contrib, there's gonna be other places where only underscore -> hyphen conversion is needed and needed quickly (for example, template_preprocess_field()), and it would help code readability and greppability to still call drupal_html_class(). So, what do you think of this? If you don't like it, feel free to re-upload #6 and mark it RTBC.

Benchmarking front page with 10 article teasers:
HEAD: 82.46ms
Patch #6: 81.96ms
Patch #9: 82.00ms

There's definitely other pages with MANY more calls to template_preprocess(), but even for this simple page, there's >0.5% savings with either #6 or #9, and not much difference between the two.

Updating title and tags for #9.

Personally, I think this extra parameter is a little bit of a hack that degrades the DX.

Yes it is a tiny hack, but I think it is a bit much to say that it degrades DX. No developer outside of a core theme system hacker has to care about this. This is just the sort of hack that you have to accept in order to speed up core. My .02.

Agreed. Bad DX is atrocities like hook_mail(), not optional parameters to largely internal functions.

Tiny, self-contained, documented hacks which only a handful of people will ever see but speed things up are completely harmless. Needless CPU usage on tens of thousands of servers running Drupal sites isn't. We don't have a lot of space to reduce PHP overhead for Drupal 7 despite it being so poor, so we need to do what we can.

spam sucks (#15,#16).

Leaving for Dries to respond to this one.

errr, an additional confusing argument for 0.04 ms? Are you serious, guys?

Patch #6: 81.96ms
Patch #9: 82.00ms

we said before that these benchmarks are done on a very simple page. they will be more favorable on real pages that theme the value every cell in a table (Views) or list lots of username and so on.

Re #19: #6 and #9 are essentially equivalent performance-wise. Either one vs. HEAD is what's important (>0.5% on a simple page, more on complex pages as Moshe points out). Personally, I think #9 is better than #6 in terms of code readability, especially if we find places other than template_preprocess() that can benefit from the same optimization (and we most certainly will).

ok, let's do this.

@Dries, #12:

Personally, I think this extra parameter is a little bit of a hack that degrades the DX.

This patch changes the new parameter name from $underscores_only to $is_machine_name. Does that make it feel less hacky?

The reason why drupal_html_class() wasn't originally included in template_preprocess() was because of the performance issues. I'm glad the bug (whoops!) was fixed in #5 above and I am totally cool with the added parameter. It means we can use a drupal_html_class() in more places without killing performance.

In fact, here's eff's patch with a few more places using the new is_machine_name parameter. I've also added a test for the new parameter.

And here's the patch where the test that checks the $is_machine_name parameter is no longer broken and actually uses said parameter. :-p

Bah. Attached the wrong patch in #26. This is the one I meant.

Ready to fly.

If we don't do this version of the patch, then we should go through and convert all the places that need the optimization to strtr(), per #6.

I know this is already RTBC, but we could we get one further change into this issue? Right now the template_preprocess() function is super-aggressive when it comes to creating these variables. Could we wrap them in an isset() check?

In template_preprocess():

   // Initialize html class attribute for the current hook.
-  $variables['classes_array'] = array(drupal_html_class($hook));
+  if (!isset($variables['classes_array'])) {
+    $variables['classes_array'] = array(drupal_html_class($hook, TRUE));
+  }

In theme.inc template_process()...

   // Flatten out classes.
-  $variables['classes'] = implode(' ', $variables['classes_array']);
+  if (!isset($variables['classes'])) {
+    $variables['classes'] = implode(' ', $variables['classes_array']);
+  }

BTW, what's up with a function named template_preprocess() and one with template_process()? I was literally looking for 5 minutes to distinguish the difference between the two names.

To elaborate on the problem in #33, I actually had a template that used a variable called "classes", that wasn't even related to CSS at all. It was literally an array of courses (as in for teaching). But because template_preprocess() runs on all templates, it kept blowing it away with the name of the template instead. I appreciate the "help" the theme system was providing, but it was a downright baffling experience trying to figure out what was happening to my variables.

@quicksketch (re #33/#34): Let's discuss that in a separate issue, please. Please open one, post a patch, and link from here.

what's up with a function named template_preprocess() and one with template_process()?

The D7 theme() function preprocesses variables in two phases. It does all the *_preprocess()/*_preprocess_HOOK() functions first, as in D6. Then, new in D7, a 2nd phase where it calls *_process()/*_process_HOOK() functions. I'm not sure that "process" was the best choice for the name of the 2nd phase, but "postpreprocess" or "latepreprocess" have their own ugliness too. Too late for changing the naming approach for D7, but let's improve it for D8.

I'll wait for this issue to complete. I can't post a patch with the overlap. In the mean time I've just renamed the variable to "courses".

#28: template_preprocess_class-680022-28.patch queued for re-testing.

It's been a long time since it was rolled. Let's see if it still applies/passes.

#28: template_preprocess_class-680022-28.patch queued for re-testing.

*Retesting old patches*

old again..

#28: template_preprocess_class-680022-28.patch queued for re-testing.

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

This is a performance improvement not affecting external APIs. Could have been committed months ago. If the testbot still likes it, it's RTBC for D7.

#28: template_preprocess_class-680022-28.patch queued for re-testing.

And it still passes. The test is a little ... weird, but back to RTBC. This is a performance enhancement that does not endanger any code, in core or contrib (if the extra parameter isn't passed, it works just the same as before). So a _{tiny API extension} that rfay can decide if it even gets a mention ;-)

Do we need to revisit this in light of #902644: Machine names are too hard to implement. Date types and menu names are not validated?

#28: template_preprocess_class-680022-28.patch queued for re-testing.

#28: template_preprocess_class-680022-28.patch queued for re-testing.

1 failure in DrupalHTMLIdentifierTestCase

Last word on this patch was that Dries didn't like it, so once again leaving for The Spikey Haired One.

(Though I agree that it seems hackish, and it would also introduce an API change for people calling drupal_html_class() in order to take advantage of a 0.5% performance benefit outlined in #9. Not sure it's really worth it at this point, honestly.)

Tagging issues not yet using summary template.

#28: template_preprocess_class-680022-28.patch queued for re-testing.

Creating an Issue summary for a concise overview of this issue

This issue seems to be completely outdated because of these changes:

https://www.drupal.org/node/2121713
https://www.drupal.org/node/1305882

The issue title is also confusing because it doesn't touch template_preprocess() function anymore. Reopen if you disagree with my decision to close!