Problem/Motivation

drupal_html_class() is called often enough that it can benefit from static caching.

Proposed resolution

Add static caching to drupal_html_class().

Remaining tasks

Profiling

User interface changes

n/a

API changes

n/a

#1982018: [meta] Refactor template_preprocess()
#1757550: [Meta] Convert core theme functions to Twig templates

CommentFileSizeAuthor
#23 0002-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch686 bytesFreekyMage
#17 0001-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch1.04 KBfabianx
#13 0001-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch1015 bytesfabianx
#5 0001-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch1.17 KBfabianx
#1 1982020-1.patch885 bytesstar-szr

Comments

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 1982020-1.patch885 bytes

Here is the relevant part of @Fabianx's patch from #1938430-16: Don't add a default theme hook class in template_preprocess().

star-szr’s picture

star-szr
he/him
Primary language English
commented

Tested with 50 nodes on the homepage.

=== 8.x..drupal_html_class-1982020 compared (517dc5e24c800..517dc6ae6625d):

ct  : 196,289|195,196|-1,093|-0.6%
wt  : 774,344|768,798|-5,546|-0.7%
cpu : 733,063|729,388|-3,675|-0.5%
mu  : 19,878,280|19,886,712|8,432|0.0%
pmu : 20,257,360|20,267,152|9,792|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517dc5e24c800&...

fabianx’s picture

fabianx commented

If this was not my patch, I would so RTBC this ... (though I really just implemented the standard cache pattern here).

twistor’s picture

twistor commented
Status: Needs review » Needs work 
+++ b/core/includes/common.incundefined
@@ -3116,7 +3116,19 @@ function drupal_clean_css_identifier($identifier, $filter = array(' ' => '-', '_
+  if (!isset($classes)) {
+    $classes = array();

Can't we just use the default argument to Drupal static, and avoid this check?

fabianx’s picture

fabianx commented
StatusFileSize
new 0001-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch1.17 KB

#4: Good idea. Implemented in this new patch.

fabianx’s picture

fabianx commented
Status: Needs work » Needs review

Get to work, Testbot!

Status: Needs review » Needs work
Issue tags: -Performance, -Twig

The last submitted patch, 0001-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch, failed testing.

thedavidmeister’s picture

thedavidmeister commented
Status: Needs work » Needs review
Issue tags: +Performance, +Twig

#5: 0001-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch queued for re-testing.

thedavidmeister’s picture

thedavidmeister commented

If this comes back green (it should) then I'm happy to RTBC this, the change is small and the pattern being implemented here is well documented.

thedavidmeister’s picture

thedavidmeister commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

This should just be a regular static, there's no way the output of this function will ever change because the static was cleared.

Also I preferred the other issue about not creating default classes at all - why add the static cache rather than cut down on the number of calls?

fabianx’s picture

fabianx commented

#11: Right!

We still create classes for node template for example, so drupal_html_class is still called enough to varant a static cache (50 times for 50 nodes for example).

The other issue might or might not land. This one does not hurt and improves performance.

Re-rolling ...

fabianx’s picture

fabianx commented
Status: Needs work » Needs review
StatusFileSize
new 0001-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch1015 bytes

And a new patch with only normal static like proposed in #11.

Status: Needs review » Needs work
Issue tags: -Performance, -Twig

The last submitted patch, 0001-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
Issue tags: +Performance, +Twig

#13: 0001-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch queued for re-testing.

thedavidmeister’s picture

thedavidmeister commented
Status: Needs review » Reviewed & tested by the community

looks good, even simpler than the last patch.

fabianx’s picture

fabianx commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 0001-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch1.04 KB

Re-rolled with better comment.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

X-POST, I only changed the wording of the comment per Cottsers request.

star-szr’s picture

star-szr
he/him
Primary language English
commented

RTBC +1 :)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Category: bug » task

+1, but not a bug

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

David_Rothstein’s picture

David_Rothstein commented
Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

Worth backporting, perhaps?

FreekyMage’s picture

FreekyMage commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 0002-Issue-1982020-by-Cottser-Fixed-Add-static-caching-to.patch686 bytes

Ported the patch.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks @FreekyMage.

David_Rothstein’s picture

David_Rothstein commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/08c9538

Status: Fixed » Closed (fixed)

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