Consider dropping date value caching

[indigoxela]

Description of the task

Date fields have an option to "Cache dates" ("Date objects can be created and cached as date fields are loaded rather than when they are displayed to improve performance.")

This value caching - at least the help text - has caused some confusion in the past. See #4249.

Personally, I've never used it (neither in D nor in B).

Recently it has been dropped completely in D7, their issue is https://www.drupal.org/node/3241762 Their reason: It causes problems (with user timezones), adds unnecessary complexity, and isn't worth it, anyway.

This simplification/cleanup looks like something to also consider for Backdrop CMS, where the date fields are in core. Do we want that?

[indigoxela]

Personally, I don't think that this date value cache provides any benefit in Backdrop. Remember, the entity cache is in core.

A screenshot of the field setting:

[olafgrabienski]

I've also never used this option, as it was not clear to me if it has real advantages. It would be interesting to hear about the option from people who use it.

[yorkshire-pudding]

I've never used it and never really understood how it could have any impact on performance.

[herbdool]

I've never used, also found it confusing. Let's drop it.

[stpaultim]

I've never used it and would agree it's a bit confusing and probably unnecessary.

1) Can we think of any backward compatibility issues with removing it? 2) Would we simply deprecate it, remove it on new sites only, or simply remove it entirely? 3) I'm not advocating for this necessarily, but could we add a telemetry check first to see if other people are using it? (Again, this may not be useful or necessary, it was just an idea).

I don't object to simply removing it if we can do it safely. But, I don't know enough about possible implications to advocate for it either.

[herbdool]

Given that Drupal 7 is pretty strict about backwards compatibility. At least in core and major contrib modules like date, I think it would be safe to remove it. Someone could see if there's a performance difference but we might not notice much with entity cache in core.

[hosef]

For the record this feature was added here. https://www.drupal.org/node/1358790

I have not used this feature either, but it seems like generating the date objects could cause significant performance problems on sites with hundreds of dates to display. I think this might be more of a problem for sites that are trying to implement scheduling systems.

[yorkshire-pudding]

Thanks for the link @hosef - in comment 72 a user called babbage suggests clearer wording:

In order to display a date, a date object is created. Creating and caching date objects at the time date field data is entered may improve performance. For single-value, non-repeating, date fields caching will be neutral or have a performance benefit. For multiple-value or repeating dates with a large number of values, caching may negatively impact performance as some date objects that are cached may never actually be required.

If we don't remove, should we consider making it clearer along the lines of this text?

[quicksketch]

I've always wanted to remove this setting too. It's esoteric and I've never used it either.

I did some benchmarking with the following set up:

• Create a new content type with 3 date fields with caching disabled.
• Generate 500 nodes of this new type.
• Make a view that displays a table of these 3 date fields.
• Load the view 5 times, recording with XHProf.

I found that in order for caching to work, it needs to be enabled at the time of content creation. So to test the cached version:

• Delete all 500 nodes.
• Enable caching on the 3 date fields.
• Generate 500 new nodes, now with caching on.
• Load the view 5 times, recording with XHProf.
• Compare before and after runs.

With caching enabled, speed on these pages is about 30% faster overall. Note this is displaying 1500 dates on a single page.

So overall, it seems like the benefit here is real. This is with core's entity caching enabled (as it is by default). Caching an entity's values isn't the same thing as caching the date objects (which are initialized and created from those values).

At the same time, there's no doubt that if we were to have ported Date module after this feature was removed, we would not put in the effort to restore such a weird option. We only have it because it was there when we ported Date module into core.

I think we should follow Drupal 7 in removing this option, despite its performance benefits.

[indigoxela]

... this is displaying 1500 dates on a single page ... So overall, it seems like the benefit here is real.

Interesting, many thanks for providing those benchmarks!

I'd like to note that 1500 dates on a single page are rather an edge-case. Even for event pages, there might not be more than 20 or maybe 50 or so. 500 nodes per views page are pretty uncommon, as well. So it might be that the performance benefit is only noticeable with unrealistic setups.

[hosef]

I think if you had a site with lots of complex components with many dates rendered, you might actually be better of with render caching instead of caching at the date field level.

5786

[herbdool]

I agree about removing this. In absolute terms the date caching shaves .3 seconds off for 1500 dates on a page. For 500 dates it might be .1 seconds but even that is an unlikely scenario. So more likely 100 dates or lower: 0.02 seconds.

[klonos]

Are we talking about removing the confusing option from the admin UI, or removing the feature altogether? If the later, then perhaps at least consider moving the functionality to a contrib date_caching module for those edge-case sites that rely on that performance gain.

[indigoxela]

Are we talking about removing the confusing option from the admin UI, or removing the feature altogether?

@klonos we consider to drop the whole thing - just as D7 did.

[olafgrabienski]

we consider to drop the whole thing - just as D7 did.

Hm, is it okay to remove the option for people who enabled it on purpose? While in Drupal it's part of a contributed module, in Backdrop it is a core feature, and just removing it would break backwards compability. What about removing it only for new installations (and improving the description for the rest)?

[indigoxela]

Hm, is it okay to remove the option for people who enabled it on purpose?

@olafgrabienski that's what we try to evaluate here. :wink:

While in Drupal it's part of a contributed module...

I'm not convinced that this makes any difference. In D7 the Date module is in contrib, but it's an important and widely used one, which is why it's probably treated cautiously like a core module, anyway.

[kiamlaluno]

Is there a procedure to follow for deprecated settings?

[indigoxela]

Is there a procedure to follow for deprecated settings?

I think so, but am not sure. It would have to happen in a minor release (no bugfix release) and it needs a changelog entry.

If we decide here to be more cautious, we could first deprecate the setting in a minor release (watchdog nagging...), and in a follow-up minor release remove the setting.

But first we need consensus. :wink:

[kiamlaluno]

I apologize: I asked to understand if this was a change that is done in a major release (for example, 2.x) or not.

[indigoxela]

I apologize: I asked to understand if this was a change that is done in a major release (for example, 2.x) or not.

No need to apologize :smiley: That's a valid question - also something we try to determine here. It might eventually be that it's a 2.x task. On the other hand nobody here seems to ever have used that setting...

[klonos]

On the other hand nobody here seems to ever have used that setting...

I know that we should most likely reserve Telemetry for more important things, but if we do need to gauge usage in order to have some actual data to back our decision with, then we should actually collect the telemetry stats before we decide. Just saying.

[stpaultim]

Discussed this in DEV meeting today. @quicksketch said that we can absolutely remove this in a backward compatible way, without changing any functionality on sites. The only exception would be that in theory, in some edge cases where a site is using this feature, their site might load slightly slower if they were using this feature AND make very heavy use of dates.

So in what seems to be very extreme edge cases, sites might load slightly smaller. I don't think that this should prevent us from removing it.

Based upon additional discussion, I'd fully support this change if anyone is inclined to work on this issue. At the same time, I still don't see this as a priority item.