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

Comments

steven jones’s picture

steven jones commented
Assigned: steven jones » Unassigned
Status: Active » Needs review
StatusFileSize
new 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 commented

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

steven jones’s picture

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 commented
StatusFileSize
new 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 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 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 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 commented

Is there any progress in this issue?

jwhat’s picture

jwhat commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 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 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
commented

#10 works for me.

cafuego’s picture

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 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 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.