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
- Optimize DrupalDateTime object creation - it seems to add quite some weight.
- 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?
Comment | File | Size | Author |
---|---|---|---|
#18 | daterefactor-1844956-18.patch | 38.84 KB | msonnabaum |
#16 | daterefactor-1844956-16.patch | 35.28 KB | Anonymous (not verified) |
#14 | daterefactor-1844956-14.patch | 34.06 KB | Anonymous (not verified) |
#11 | daterefactor-1844956-11.patch | 32.05 KB | msonnabaum |
#11 | interdiff.txt | 1.55 KB | msonnabaum |
Comments
Comment #1
sunRegarding 2), doesn't that boil down to a simple
instanceof DrupalDateTime
informat_date()
?Comment #2
KarenS CreditAttribution: KarenS commentedNote 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.
Comment #3
KarenS CreditAttribution: KarenS commentedAlso 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.
Comment #4
fagoI'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.
Comment #5
catchThis is a serious regression from Drupal 7, bumping to critical.
Comment #6
catchActually it's more major bug.
Comment #7
msonnabaum CreditAttribution: msonnabaum commentedHere'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.
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.
Comment #8
msonnabaum CreditAttribution: msonnabaum commentedComment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedlooks like one of the fails is a missing conversion to DrupalDateTime::createFromTimestamp(). in core/modules/comment/lib/Drupal/comment/CommentFormController.php, this:
should be:
Comment #11
msonnabaum CreditAttribution: msonnabaum commentedGood catch.
THis one should fix some more fails.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedthe DateTimePlusIntlTest needs this:
to be this:
core/modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusIntlTest.php
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedhere'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().
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedok, 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.
Comment #17
tim.plunkettNeeds a docblock, onliner + @var
missing @param, and we usually put
public static function
This is wrapped weird
Double blank line
Missing docblock
s/TODO:/@todo/
Tests
Bad spacing, and missing docblock
Tests, and that second line is weird
None of these are @params
Provides, here and elsewhere
Comment #18
msonnabaum CreditAttribution: msonnabaum commentedThis 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.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedi only provided bug fixes for this patch, so i think i can RTBC.
Comment #20
catchNumbers look great, as does the refactor.
Couple of whitespace issues which I fixed on commit.
This will need a change notice.
Comment #21
catchComment #22
catch#2083415: [META] Write up all outstanding change notices before release
Comment #23
martin107 CreditAttribution: martin107 commentedMissing change notice has led to DX confusion see issue #2116327.
Comment #23.0
martin107 CreditAttribution: martin107 commentedUpdated issue summary.
Comment #24
alexweber CreditAttribution: alexweber commentedGonna write this change record...
Comment #25
alexweber CreditAttribution: alexweber commentedSummary
Before
After
Comment #26
BerdirThis 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.
Comment #27
j4 CreditAttribution: j4 commentedUpdated Change record to recommend usage of the new APIs.
Comment #28
BerdirNote 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.
Comment #29
j4 CreditAttribution: j4 commentedThank 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
Comment #30
BerdirYou 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.
Comment #31
j4 CreditAttribution: j4 commentedSorry, i thought i had added it! but seem to have missed. Added it now.
Jaya
Comment #32
BerdirI guess it somehow didn't stick.
I added it now, also rewrote it a bit and updated the existing code examples.
Comment #33
Berdir