Posted by catch on May 1, 2009 at 10:11pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
http://api.drupal.org/api/function/template_preprocess_node/7
and
http://api.drupal.org/api/function/theme_node_submitted
both do format_date($node->submitted);
Those 20 calls to format_date() on a default front page account for 5.5% of execution time.
We should either split $submitted into author and date, or remove $date and let themes handle it themselves - but doing this work twice when we only use one variable in any one template is a waste.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| format_date.png | 63.29 KB | Ignored: Check issue status. | None | None |
Comments
#1
Let's try just removing it first. There must be an existing issue somewhere for making $submitted less horrible.
10 nodes on front page. ab -c1 -n500
HEAD:
8.92 [#/sec]
8.80 [#/sec]
9.20 [#/sec]
Patch:
9.37[#/sec]
9.40 [#/sec]
9.46 [#/sec]
Benchmarks come out more like 6% compared to the expected 2.5% from profiling, but close enough and it's definitely a measurable cost.
#2
Comments have the same problem.
#3
Nice work focusing on this function. Why is it such a pig? I would have thought that the new Date API in PHP is fast.
Somehow, we need to get to a pull system where we only prepare what the theme needs.
#4
Wow, that is a surprise. This sounds like RTBC to me, but I agree that a pull system would probably by desired at some point.
#5
#243129: Refactor format_date() to avoid multiple calls to date_format() has been around or a while but format_date() has changed significantly since then so it may not be the same issue any more.
I thought about adding a static cache to format_date() as well - would like to try that before we remove the variable.
#6
Here's a try with a static. Didn't seem to be any need to use drupal_static() here - can't see the static needing to be cleared but I might've overlooked something.
Attached webgrind screenshots.
#7
#243129: Refactor format_date() to avoid multiple calls to date_format() is still needed. Instead of adding a static in format_date(), couldn't we avoid the duplicate call inside the field api?
#8
It's not field API though - it's template_preprocess_node/comment creating their own date variables as well as using the _submitted theme functions.
Removing $date from template_preprocess_node() (and the same with comments) removes the duplication in HEAD - but I can see themes adding that back in again all over the place,. Since Drupal 6 has the same issue, we could potentially backport a static (300 extra calls to format_date() on a long thread is potentially enough to warrant it).
Ideal thing for HEAD would be for format_date() to not be such a beast, but no-one's working on that issue and I'm not really up on date handling enough to tackle it, so it's a case of whether we do something interim or try to focus efforts over there.
#9
Yeah, themes are always wanting to do different date formats because every site owner wants their own bikeshed.
Since that new static cache patch landed, this patch needs to be re-worked to use that method, no? /me admits to being too sleepy to look it up at the 'mo. Leaving at "needs review" for now.
#10
Postponing this on these two:
#243129: Refactor format_date() to avoid multiple calls to date_format()
#216961: Make author/submitted by separate toggles
#11
Un-postponing. @catch: what's your current thinking on this?
#12
#6: format_date.patch queued for re-testing.
#13
The last submitted patch, format_date.patch, failed testing.