Problem/Motivation

Since #1605290: Enable entity render caching with cache tag support has landed (and the related #2099131: Use #pre_render pattern for entity render caching), we render cache entire rendered entities. We vary them by theme, user role and — if translated — by translation.

But we don't yet vary by time zone! This means that if a person in western Europe (CET) would have looked at a node, then the rendered time would say e.g. "17:00", but when a person from the US east coast would then look at it, the same time would be displayed, instead of "11:00".

Much thanks to Berdir for pointing this out!

Proposed resolution

  1. Add a cache.context.timezone (TimeZoneCacheContext) service.
  2. Make EntityViewBuilder::getBuildDefaults() use this.

Remaining tasks

Review.

User interface changes

None.

API changes

Added a cache.context.timezone (TimeZoneCacheContext) service.

CommentFileSizeAuthor
#13 interdiff.txt2.65 KBwim leers
#13 entities_render_cache_by_timezone-2274791-13.patch4.92 KBwim leers
#9 entities_render_cache_by_timezone-2274791-9.patch2.33 KBwim leers
#4 interdiff.txt824 byteswim leers
#4 entities_render_cache_by_timezone-2274791-4.patch4.61 KBwim leers
#1 entities_render_cache_by_timezone-2274791-1.patch4.47 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Rendered entities should be cached by timezone » Rendered entities should be cached by time zone
Status: Active » Needs review
Issue tags: +Spark, +sprint
StatusFileSize
new entities_render_cache_by_timezone-2274791-1.patch4.47 KB

Status: Needs review » Needs work

The last submitted patch, 1: entities_render_cache_by_timezone-2274791-1.patch, failed testing.

yched’s picture

yched commented

Naive question :

This unconditionally splits render caches by timezones for all entities. In many cases (e.g teasers that do not display a date), that's overkill, and adds unneeded cache misses.

Is there a way this could be triggered only if a rendered formatter introduces timezone considerations in the displayed entity ?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
Related issues: +#2099137: Entity/field access and node grants not taken into account with core cache contexts
StatusFileSize
new entities_render_cache_by_timezone-2274791-4.patch4.61 KB
new interdiff.txt824 bytes

#3:

I hoped for that question :)

First: teasers actually do show a timezone-dependent time, e.g. "Submitted by Anonymous (not verified) on Sat, 05/24/2014 - 17:04".

Second: yes, this should be added by a field formatter. But that's currently not yet possible, for two reasons:

  1. the API needs to allow for that, but doesn't yet — this is being worked on over at #2099137: Entity/field access and node grants not taken into account with core cache contexts
  2. this means that everything in every entity must be rendered by entity display components, and unfortunately many base fields still have hardcoded rendering (#2226493: Apply formatters and widgets to Node base fields in particular, but also the related issues for other entity types)

So: yes, eventually that will be the case, but for now, we unfortunately have to do it like this for now. I added a @todo.

Status: Needs review » Needs work

The last submitted patch, 4: entities_render_cache_by_timezone-2274791-4.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Since rendering a time in a timezone is very cheap, would this perhaps be a candidate for #post_render_cache? I.e. a placeholder with a Unix timestamp should suffice.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#6: No, because there are usually dozens of timestamps on the page. We should keep #post_render_cache callbacks only for those things are 1) not numerous, 2) would cause a quasi-limitless number of render cache items. There are only a few dozen possible timezones, therefore varying by timezone is most sensible.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Thanks for that explanation. Makes a lot of sense!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new entities_render_cache_by_timezone-2274791-9.patch2.33 KB

The above patches trigger an infinite loop :)

When a page is requested, the user and session objects are constructed, and at the end of SessionManager::initialize(), date_default_timezone_set(drupal_get_user_timezone()) is called. But drupal_get_user_timezone() uses UserTimeZoneCacheContext in the above patches, which calls $this->user->isAuthenticated(), but the user object is not yet initialized, so it ends up going to SessionManager::initialize() again, etc.

Plus, it was technically even wrong, because it's possible that another module called date_default_timezone_set(), and then the above would've yielded wrong results.

This new approach is simpler and should always be correct.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 9: entities_render_cache_by_timezone-2274791-9.patch, failed testing.

znerol’s picture

znerol commented

Test:


date_default_timezone_set('Europe/Amsterdam');

$dates = array();

$t0 = microtime(TRUE);
for ($i = 0; $i < 1000; $i++) {
  $stamp = mt_rand(0, time());
  $date = new DateTime();
  $date->setTimestamp($stamp);
  $dates[] = $date;
}

$t1 = microtime(TRUE);

foreach ($dates as $date) {
  $date->format(DateTime::ISO8601);
}
$t2 = microtime(TRUE);

print "Construct objects: [s]: " . ($t1 - $t0) . "\n";
print "Format date:       [s]: " . ($t2 - $t1) . "\n";

Result:

$ php test.php 
Construct objects: [s]: 0.0075211524963379
Format date:       [s]: 0.0015008449554443

I think that this modest performance penalty introduced by a #post_render_cache pass is justifiable. Even more because there is still the option to offload this kind of processing to JavaScript.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new entities_render_cache_by_timezone-2274791-13.patch4.92 KB
new interdiff.txt2.65 KB

This should be green.

#12: The biggest cost will not be the building of the date, but the fact that the date itself may be *themed* (invoking the theme layer). Plus, even if it were cheap, generating 1000 render cache placeholders and searching them in the dozens of kilobytes worth of HTML… that doesn't seem too appealing.

Standardizing on #post_render_cache to render every date that is timezone specific is counter to the reason it was added to Drupal core: for the one-offs that are not cacheable. Rendering per timezone is easily cacheable: the N remains low.

Let us please not do that.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Agreed. Also keep in mind that many sites will not have configurable user time zones and one more cache tag to check per page isn't going to hurt them much but if date rendering would be a post render cache callback, they would have to run all that additional complexity as well.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

So… RTBC? :)

moshe weitzman’s picture

moshe weitzman commented

I'm not particularly happy about cache misses going up by 23x for entities and anything else that uses this context. But I can't think of a better approach.

I know that my statement only applies to sites that allow user configurable timezones. Thats a small consolation.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#16: Indeed. Except that very, very, very few sites are actually visited by authenticated users from 24 different timezones (because this will also only affect authenticated users, anonymous users always get the site's timezone).

Note that *if* we were to do this in JS, then:

  1. nobody may ever render a time or date without going through this new API
  2. it would not work from within the theme layer, because in the theme layer (templates, theme functions and preprocess functions) it's technically impossible to attach assets
  3. that JS would need to support 1) translationss, 2) any crazy time formatting that PHP's date() function supports… (To give you an example, "Monday" is "maandag" in Dutch. The short version that PHP prints is supposedly a 3-letter abbreviation, which is "Mon" in English, yet it prints "ma" in Dutch. How are we possibly going to make all of that work correctly?)

Therefore, let us please get this patch in now, so that at least what we have is correct. We can then worry about improving performance.

moshe weitzman’s picture

moshe weitzman commented

Yes, javascript is out. I think the only alternative solution is #post_render_cache. I'm OK with committing this for now. Curious what catch and msonnabaum think.

catch’s picture

catch
he/him
Primary language English
commented

I think I agree with Wim that in practice the cache won't be divided by an additional 24. Additionally peak traffic from particular timezones is likely to be concentrated at similar time periods during the day.

Could we avoid applying this if 'as time ago' formatting is being used though? That's not affected by timezone. Could be a follow-up if it's tricky.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#19: Yes, that would work, but we can't quite do that yet:

  • yched in #3:

    Is there a way this could be triggered only if a rendered formatter introduces timezone considerations in the displayed entity ?

  • My reply in #4:

    yes, this should be added by a field formatter. But that's currently not yet possible, for two reasons:

    1. the API needs to allow for that, but doesn't yet — this is being worked on over at #2099137: Entity/field access and node grants not taken into account with core cache contexts
    2. this means that everything in every entity must be rendered by entity display components, and unfortunately many base fields still have hardcoded rendering (#2226493: Apply formatters and widgets to Node base fields in particular, but also the related issues for other entity types)
  • So: for now we can't do this, because we can't know if an entity view builder is generating timestamps also, nor can we know about timestamps from the theme layer. Plus, you can't use "time ago" formatters for every use case. So, this is the general solution, we can work on optimizing the narrower "time ago" use case in a follow-up.
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Agreed, We can look into optimizing it but I'm not too worried about this, the amount of timezones of most sites is limited to one or a few and it's always the same for anonymous users.

Note that this adds a new "Time Zone" selection to the block caching settings, I guess that's OK and can't be avoided atm anyway.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#21: Correct, every request cache context is automatically exposed in a cacheable block's cache settings.

Thanks! :)

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -174,6 +174,9 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
+          // @todo Move this out of here and into field formatters that depend
+          //       on the timezone. Blocked on ghttps://drupal.org/node/2099137.
+          'cache_context.timezone',

ghttps.

Fixed on commit.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -sprint

Thanks!

  • Commit 2405d10 on 8.x by catch: 
    Issue #2274791 by Wim Leers: Fixed Rendered entities should be cached by...

Status: Fixed » Closed (fixed)

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