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?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

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

KarenS’s picture

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.

KarenS’s picture

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.

fago’s picture

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.

catch’s picture

Priority: Normal » Critical
Issue tags: +Performance

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

catch’s picture

Category: task » bug
Priority: Critical » Major

Actually it's more major bug.

msonnabaum’s picture

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

msonnabaum’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

Anonymous’s picture

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

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

should be:

      $date = (!empty($comment->date) ? $comment->date : DrupalDateTime::createFromTimestamp($comment->created->value));
msonnabaum’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
32.05 KB

Good catch.

THis one should fix some more fails.

Status: Needs review » Needs work

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

Anonymous’s picture

the DateTimePlusIntlTest needs this:

    $intl_date = new DateTimePlus($input, $timezone, NULL, $intl_settings);
    $php_date = new DateTimePlus($input, $timezone, NULL, $php_settings);

to be this:

    $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

Anonymous’s picture

Status: Needs work » Needs review
FileSize
34.06 KB

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.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
35.28 KB

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.

tim.plunkett’s picture

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

msonnabaum’s picture

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.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Title: Optimize date formatting performance » Change 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.

catch’s picture

Issue tags: +Needs change record
catch’s picture

martin107’s picture

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

martin107’s picture

Issue summary: View changes

Updated issue summary.

alexweber’s picture

Assigned: Unassigned » alexweber
Issue summary: View changes

Gonna write this change record...

alexweber’s picture

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

$date = new DrupalDateTime($date);

$intl_date = new DateTimePlus($input, $timezone, NULL, $intl_settings);
$php_date = new DateTimePlus($input, $timezone, NULL, $php_settings);

After

$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);
Berdir’s picture

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.

j4’s picture

Updated Change record to recommend usage of the new APIs.

Berdir’s picture

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.

j4’s picture

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

Berdir’s picture

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.

j4’s picture

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

Jaya

Berdir’s picture

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.

Berdir’s picture

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

Status: Fixed » Closed (fixed)

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