While optimizing performance for #1778178-88: Convert comments to the new Entity Field API berdir noted that date formatting performance is slow due to DrupalDateTime objects being created:

Many calls like this one: format_date($comment->created->value->getTimestamp()). We already discussed this before, but what happened in the meantime is that format_date() was converted to using DrupalDateTime, so it does internally a new DrupalDateTime($timestamp)->format() (conceptually). Which means we convert from DateTime to timestamp and then back into a DrupalDateTime object. Not necessarly here, but we really should change our TypeData Date class to *somehow* use DrupalDateTime() (extend or wrap, not sure) and avoid this.

Right now the TypedData date class already is a DrupalDateTime instance. I noted that creating this DrupalDateTime instances slowed down things, so I tried going with DateTime instead, giving that results:

before: 248ms for 996 calls
after:   31ms for 996 calls

Then, I reverted this and tried avoiding format_date() in favour of leveraging the already existing object, i.e. use $comment->created->value->format(variable_get('date_format_medium', 'D, m/d/Y - H:i'));.

Results:

before: 441ms Date formatting with date_format()
after:  198ms (996 calls)

So the latter gives a slightly better performance boost + keeps using the DrupalDateTime class. So maybe we can

  1. Optimize DrupalDateTime object creation - it seems to add quite some weight.
  2. Have a format() method that accepts Drupal formats as format_date(), such that re-using the object works in a sane way. Imho we could even remove format_date() in favour of $date->format().

Thoughts?

Files: 
CommentFileSizeAuthor
#18 daterefactor-1844956-18.patch38.84 KBmsonnabaum
PASSED: [[SimpleTest]]: [MySQL] 58,105 pass(es).
[ View ]
#16 daterefactor-1844956-16.patch35.28 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 57,628 pass(es).
[ View ]
#14 daterefactor-1844956-14.patch34.06 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] 57,892 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#11 daterefactor-1844956-11.patch32.05 KBmsonnabaum
FAILED: [[SimpleTest]]: [MySQL] 57,820 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#11 interdiff.txt1.55 KBmsonnabaum
#7 daterefactor-1844956-7.patch30.15 KBmsonnabaum
FAILED: [[SimpleTest]]: [MySQL] 57,555 pass(es), 7 fail(s), and 4 exception(s).
[ View ]
#7 Screen Shot 2013-08-03 at 17.09.00 .png101.31 KBmsonnabaum
#7 Screen Shot 2013-08-03 at 17.08.55 .png66.11 KBmsonnabaum

Comments

Regarding 2), doesn't that boil down to a simple instanceof DrupalDateTime in format_date()?

Note that this behavior will be improved by #501428: Date and time field type in core, if it gets in. In that patch we create a date object and pass it to the form elements so we don't have to keep creating a new date object at each stage of form processing.

Also the changes in #1571632: Convert regional settings to configuration system affect this as well. It would help if we could get some of these other patches in and then focus on where things stand.

Regarding 2), doesn't that boil down to a simple instanceof DrupalDateTime in format_date()?

I'm not sure. format_date() also has some timezone handling that is probably deprecated then also, so maybe we want to have two functions or just have it in a method on the object? format_date() then can call the method also.

Priority:Normal» Critical
Issue tags:+Performance

This is a serious regression from Drupal 7, bumping to critical.

Category:task» bug
Priority:Critical» Major

Actually it's more major bug.

StatusFileSize
new66.11 KB
new101.31 KB
new30.15 KB
FAILED: [[SimpleTest]]: [MySQL] 57,555 pass(es), 7 fail(s), and 4 exception(s).
[ View ]

Here's an initial stab at this.

The attached patch does a few different things to improve performance, mostly just avoiding function calls to config and language where possible, and caching the check for IntlDateFormatter, since that won't ever change during a request (it was triggering the autoloader every time).

I ended up refactoring DateTimePlus a bit more than I had planned, but the way it was designed was very difficult to refactor. Having a constructor that can take so many different versions of the arguments just isn't helpful, and comes at the cost of significant complexity, so I just made factory methods for each type.

Here is the before/after on a node page with 50 comments.

Screen Shot 2013-08-03 at 17.08.55 .pngScreen Shot 2013-08-03 at 17.09.00 .png

We could probably shave off another ~10ms from that if we could avoid the calls to drupal_get_user_timezone, but I didnt have a great idea for it atm.

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, daterefactor-1844956-7.patch, failed testing.

looks like one of the fails is a missing conversion to DrupalDateTime::createFromTimestamp(). in core/modules/comment/lib/Drupal/comment/CommentFormController.php, this:

<?php
      $date
= (!empty($comment->date) ? $comment->date : new DrupalDateTime$comment->created->value));
?>

should be:

<?php
      $date
= (!empty($comment->date) ? $comment->date : DrupalDateTime::createFromTimestamp($comment->created->value));
?>

Status:Needs work» Needs review
StatusFileSize
new1.55 KB
new32.05 KB
FAILED: [[SimpleTest]]: [MySQL] 57,820 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Good catch.

THis one should fix some more fails.

Status:Needs review» Needs work

The last submitted patch, daterefactor-1844956-11.patch, failed testing.

the DateTimePlusIntlTest needs this:

<?php
    $intl_date
= new DateTimePlus($input, $timezone, NULL, $intl_settings);
   
$php_date = new DateTimePlus($input, $timezone, NULL, $php_settings);
?>

to be this:

<?php
    $intl_date
= new DateTimePlus($input, $timezone, $intl_settings);
   
$php_date = new DateTimePlus($input, $timezone, $php_settings);
?>

core/modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusIntlTest.php

Status:Needs work» Needs review
StatusFileSize
new34.06 KB
FAILED: [[SimpleTest]]: [MySQL] 57,892 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

here's another go that might fix the remaining fails. apart from the fix in #13, it also adds an is_numeric() check to DateTimePlus::createFromTimestamp().

Status:Needs review» Needs work

The last submitted patch, daterefactor-1844956-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new35.28 KB
PASSED: [[SimpleTest]]: [MySQL] 57,628 pass(es).
[ View ]

ok, rerolled to keep up with head, and added a couple of fixes for views arg handler tests that only worked because we accepted rubbish input to date constructors.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -105,6 +105,87 @@ class DateTimePlus extends \DateTime {
+  static $intlExtentionExists = NULL;

Needs a docblock, onliner + @var

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -105,6 +105,87 @@ class DateTimePlus extends \DateTime {
+   * Creates a date object from an input date object.
+   */
+  static public function createFromDateTime(\DateTime $datetime, $settings = array()) {
...
+   * Creates a date object from an array of date parts.
+   *
+   * Converts the input value into an ISO date, forcing a full ISO
+   * date even if some values are missing.
+   */
+  static public function createFromArray(array $date_parts, $timezone = NULL, $settings = array()) {
...
+   * Creates a date object from timestamp input.
+   *
+   * The timezone of a timestamp is always UTC. The timezone for a
+   * timestamp indicates the timezone used by the format() method.
+   */
+  static public function createFromTimestamp($timestamp, $timezone = NULL, $settings = array()) {
...
+   * Creates a date object from an input format.
+   */
+  static public function createFromFormat($format, $time, $timezone = NULL, $settings = array()) {

missing @param, and we usually put public static function

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -105,6 +105,87 @@ class DateTimePlus extends \DateTime {
+    // Tries to create a date from the format and use it if possible.
+    // A regular try/catch won't work right here, if the value is
+    // invalid it doesn't return an exception.
...
+      // The createFromFormat function is forgiving, it might
+      // create a date that is not exactly a match for the provided
+      // value, so test for that. For instance, an input value of
+      // '11' using a format of Y (4 digits) gets created as
+      // '0011' instead of '2011'.
+      // Use the parent::format() because we do not want to use
+      // the IntlDateFormatter here.

This is wrapped weird

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -105,6 +105,87 @@ class DateTimePlus extends \DateTime {
+    $datetimeplus = new static('', $timezone, $settings);
+
+
+    $date = \DateTime::createFromFormat($format, $time, $datetimeplus->getTimezone());

Double blank line

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.phpundefined
@@ -655,7 +525,14 @@ public function canUseIntl($calendar = NULL, $langcode = NULL, $country = NULL)
+  public static function intlDateFormatterExists() {
+++ b/core/lib/Drupal/Core/Datetime/Date.phpundefined
@@ -39,6 +39,9 @@ class Date {
+  protected $country = NULL;
+  protected $dateFormats = array();
@@ -120,4 +129,18 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
+  protected function dateFormat($format) {
...
+  protected function country() {

Missing docblock

+++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/field/field_type/DateTimeItem.phpundefined
@@ -122,9 +122,14 @@ public function prepareCache() {
+        // TODO: Handle this.

s/TODO:/@todo/

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -51,6 +51,28 @@ public function testDates($input, $timezone, $expected) {
+   * Test creating dates from string and array input.

Tests

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -76,12 +98,25 @@ public function testDates($input, $timezone, $expected) {
+  }
+
+  public function assertDateTimestamp($date, $input, $initial, $transform) {

Bad spacing, and missing docblock

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -172,6 +207,31 @@ public function testDateTimezone($input, $timezone, $expected_timezone, $message
+   * Test that DrupalDateTime can detect the right timezone to use.
+   * When specified or not.

Tests, and that second line is weird

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -172,6 +207,31 @@ public function testDateTimezone($input, $timezone, $expected_timezone, $message
+   * @param mixed $input
+   *   Input argument for DateTimePlus.
+   * @param mixed $timezone
+   *   Timezone argument for DateTimePlus.
+   * @param string $expected_timezone
+   *   Expected timezone returned from DateTimePlus::getTimezone::getName().
+   * @param string $message
+   *   Message to print on test failure.
+   */
+  public function testDateTimezoneWithDateTimeObject() {

None of these are @params

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -195,7 +255,20 @@ public function providerTestDates() {
+   * Provide data for date tests.
@@ -259,6 +332,24 @@ public function providerTestInvalidDates() {
+   * Provide data for testInvalidDates.

Provides, here and elsewhere

StatusFileSize
new38.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,105 pass(es).
[ View ]

This should take care of everything in #17, and also fixes a small bug I found.

Also, most of those doc fixes were from what's in core now. I'd prefer to not nitpick anything else that's not related to this patch.

Status:Needs review» Reviewed & tested by the community

i only provided bug fixes for this patch, so i think i can RTBC.

Title:Optimize date formatting performanceChange notice: Optimize date formatting performance
Category:bug» task
Priority:Major» Critical
Status:Reviewed & tested by the community» Active

Numbers look great, as does the refactor.

Couple of whitespace issues which I fixed on commit.

This will need a change notice.

Issue tags:+Needs change record

Missing change notice has led to DX confusion see issue #2116327.

Issue summary:View changes

Updated issue summary.

Assigned:Unassigned» alexweber
Issue summary:View changes

Gonna write this change record...

Assigned:alexweber» Unassigned
Status:Active» Needs review

Summary

  • format_date() was causing performance issues due to DrupalDateTime objects being created unnecessarily; dates were being received as objects, converted to timestamps and then back to objects again
  • The DateTimePlus constructor was a bit confusing because of how it handled different versions of arguments

Before

<?php
$date
= new DrupalDateTime($date);
$intl_date = new DateTimePlus($input, $timezone, NULL, $intl_settings);
$php_date = new DateTimePlus($input, $timezone, NULL, $php_settings);
?>

After

<?php
$date
= DrupalDateTime::createFromDateTime($date);
$date = DrupalDateTime::createFromArray($array);
$date = DrupalDateTime::createFromTimestamp($array);
$date = DrupalDateTime::createFromFormat($array);
$intl_date = new DateTimePlus($input, $timezone, $intl_settings);
$php_date = new DateTimePlus($input, $timezone, $php_settings);
?>

This is a HEAD to HEAD change (this class does not exist in 7.x) that already happened quite a long time ago, I don't think we need a new change notice for this.

What we need to make sure is that https://drupal.org/node/1834108 is updated to use the new API's. That should be enough.

Updated Change record to recommend usage of the new APIs.

Note that code examples should be in there, not just referenced to the issue.

You should also use the issue reference field, so that the change notice also shows up here in the issue.

Thank you for the review. Updated the change record. On editing this node, the issue reference field does not accept the Change record. IS there any other way i should reference the change record here/

Jaya

You don't need to edit this node. The change notice has a reference field, and when you add it there, it shows up here as well.

Sorry, i thought i had added it! but seem to have missed. Added it now.

Jaya

Status:Needs review» Fixed

I guess it somehow didn't stick.

I added it now, also rewrote it a bit and updated the existing code examples.

Title:Change notice: Optimize date formatting performanceOptimize date formatting performance
Issue tags:-Needs change record

Status:Fixed» Closed (fixed)

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