I have a relatively complex page, a month view calendar showing events that have repeating dates, the page was incredibly slow to load, and one of the culprits was date_limit_format, which seems to involve lots of regexes and string manipulations.

The inputs to the function are relatively simple, so I suggest using a simple static cache to only run the expensive parts of this function once per page load, per format and granularity.

CommentFileSizeAuthor
#10 date-1835184-date_limit_format-static-cache-10.patch1.17 KBjwhat
#5 date-date_limit_format-static-cache-1835184-5.patch1.15 KBdas-peter
#1 date-1835184-date_limit_format-static-cache.patch1.15 KBSteven Jones
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones commented
Assigned: Steven Jones » Unassigned
Status: Active » Needs review
FileSize
date-1835184-date_limit_format-static-cache.patch1.15 KB

Before patch, xhprof tells me that my page in question spends 213,370 microseconds (0.2 seconds!) in date_limit_format.

After the patch, xhprof tells me that my page spends 16,906 microseconds (0.016 seconds) in date_limit_format, and that about 2,500 microseconds is the overhead of adding the static caching and computing the cache keys etc.

Status: Needs review » Needs work

The last submitted patch, date-1835184-date_limit_format-static-cache.patch, failed testing.

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones commented

I'm not sure that exception has anything to do with this patch?

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones commented
Status: Needs work » Needs review

#1: date-1835184-date_limit_format-static-cache.patch queued for re-testing.

das-peter’s picture

das-peter CreditAttribution: das-peter commented
FileSize
date-date_limit_format-static-cache-1835184-5.patch1.15 KB

Just came across the same xhprof results. This is getting essential using denormalization to index repeated dates with search_api_solr.
Patch provides the boost, however I'm not sure if we really want to split the function.
How about the approach in the attached patch?

Another thing with the patch:

+++ b/date_api/date_api.module
@@ -2100,6 +2100,25 @@ function date_part_format($part, $format) {
+  $formats = &$drupal_static_fast['formats'];

Why do we need $formats and why the extra hierarchy ['formats']. We could use plain $drupal_static_fast, right?

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones commented

Why do we need $formats and why the extra hierarchy ['formats']. We could use plain $drupal_static_fast, right?

Wrong, according to: http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drupa... and: http://drupal.org/node/619666#comment-2211696 the reference stored in a static variable is lost, so we need this extra construction.

As for splitting the function, maybe it makes more sense not to, up to the maintainer I'd suggest.

das-peter’s picture

das-peter CreditAttribution: das-peter commented
Status: Needs review » Reviewed & tested by the community

Oh, I wasn't aware of that. Thanks for letting me know!
Besides the question if we should split the function I'd say #1 is RTBC.

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones commented

Yeah, PHP gets messy some times :(

Good to note that others have had performance issues with Date module and its not just me!

manuelBS’s picture

manuelBS CreditAttribution: manuelBS commented

Is there any progress in this issue?

jwhat’s picture

jwhat CreditAttribution: jwhat commented
Status: Reviewed & tested by the community » Needs review
FileSize
date-1835184-date_limit_format-static-cache-10.patch1.17 KB

I came across the same performance issues with date_limit_format and tested this patch from #1, works great. I rewrote the patch so its all in the same function now.

das-peter’s picture

das-peter CreditAttribution: das-peter commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks for the updated patch!

The only thing that came to my mind while reviewing was if we shouldn't link the variable $format right away to $formats[$format_granularity_cid].
E.g.

$formats[$format_granularity_cid] = '';
$format = &$formats[$format_granularity_cid];

That way we won't need 

  // Store the return value in the static array for performance.
  $formats[$format_granularity_cid] = $format;

And even if the code is changed to return the variable before setting $format in the cache the caching won't break.
But that's just from a paranoid defensive perspective - patch as is works and looks good :)

ckng’s picture

ckng
Primary language English
CreditAttribution: ckng commented

#10 works for me.

cafuego’s picture

cafuego CreditAttribution: cafuego commented
Status: Reviewed & tested by the community » Needs work 
+++ b/date_api/date_api.module
@@ -2100,6 +2100,17 @@ function date_part_format($part, $format) {
+  if (!isset($drupal_static_fast)) {
+    $drupal_static_fast['formats'] = &drupal_static(__FUNCTION__);
+  }
+  $formats = &$drupal_static_fast['formats'];

You're checking that $drupal_static_fast exists, but then referencing $drupal_static_fast['formats'] without checking wether that array key exists. Wouldn't that cause a PHP warning about unknown index keys?

Is there any reason to not simply use something like

if (empty($formats)) {
  $formats = &drupal_static(__FUNCTION__);
}

instead, without that extra level of array variable?

cafuego’s picture

cafuego CreditAttribution: cafuego commented
Status: Needs work » Fixed

Update: beejeebus tells me it's possibly PHP awfulness or historical raisins or that at any rate static caching makes his eyes bleed, so I'll apply the patch as-is ;-)

das-peter’s picture

das-peter CreditAttribution: das-peter commented

Awesome, thanks :) Hope beejeebus eyes get well soon and that at least you're not injured :D

Status: Fixed » Closed (fixed)

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