Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#10 | date-1835184-date_limit_format-static-cache-10.patch | 1.17 KB | jwhat |
#5 | date-date_limit_format-static-cache-1835184-5.patch | 1.15 KB | das-peter |
#1 | date-1835184-date_limit_format-static-cache.patch | 1.15 KB | Steven Jones |
Comments
Comment #1
Steven Jones CreditAttribution: Steven Jones commentedBefore 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.Comment #3
Steven Jones CreditAttribution: Steven Jones commentedI'm not sure that exception has anything to do with this patch?
Comment #4
Steven Jones CreditAttribution: Steven Jones commented#1: date-1835184-date_limit_format-static-cache.patch queued for re-testing.
Comment #5
das-peter CreditAttribution: das-peter commentedJust 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:
Why do we need
$formats
and why the extra hierarchy['formats']
. We could use plain$drupal_static_fast
, right?Comment #6
Steven Jones CreditAttribution: Steven Jones commentedWrong, 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.
Comment #7
das-peter CreditAttribution: das-peter commentedOh, 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.
Comment #8
Steven Jones CreditAttribution: Steven Jones commentedYeah, PHP gets messy some times :(
Good to note that others have had performance issues with Date module and its not just me!
Comment #9
manuelBS CreditAttribution: manuelBS commentedIs there any progress in this issue?
Comment #10
jwhat CreditAttribution: jwhat commentedI 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.
Comment #11
das-peter CreditAttribution: das-peter commentedLooks 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.
That way we won't need
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 :)
Comment #12
ckng#10 works for me.
Comment #13
cafuego CreditAttribution: cafuego commentedYou'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
instead, without that extra level of array variable?
Comment #14
cafuego CreditAttribution: cafuego commentedUpdate: 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 ;-)
Comment #15
das-peter CreditAttribution: das-peter commentedAwesome, thanks :) Hope beejeebus eyes get well soon and that at least you're not injured :D