Store date objects with raw items

Well I am all in favor of improving performance. So conceptually I'm on board. This obviously needs to be tested thoroughly.

I was going to go ahead and commit this patch to see what it does in the wild, but when I checked that all our tests passed, I found that the Date Migrate test fails. I've got to see what might be wrong there.

Can't figure out why the Migrate test fails. All the dates created by the migration process seem to have dates that are off by the amount of the site timezone adjustment. This may indicate a problem with nodes that are programmatically created with this patch.

Plus after applying the patch if I view an existing date I get a page full of notices about missing indexes. Clearing all the caches does not help. The only way to get rid of the messages is to edit and re-save the node. So it looks like the patch needs to include an update to empty the node cache or something.

In poking around at this I remember another reason I wasn't doing the timezone conversion in the load, because if we have user-specific timezones we can't let the timezone adjustment get stored in the node cache. If we want to cache the timezone conversions, we at a minimum will have to have a separate cache for each timezone. I don't know if the entity cache will let us do that -- provide two or more variations of a cached node.

Or I guess we could cache the node using the site timezone, and then do a realtime conversion for anyone using something other than the site timezone.

We still have to take into account #10. For dates that have user-specific timezones we have to skip creating the date object and create it in the theme, as I do now (or some time after the node has been retrieved from the cache). I think we have to do both, check during the load to keep those values out of the cache and check in the theme to see if we have to create the date there.

I'm open to other ideas about how to handle that, but that's the only option I can think of.

Also, nice job in figuring out what was going on with the test. That's a weird one :)

I tested this patch in combination with fullcalendar and it improves the speed A LOT. I only had small datasets available but even there the speed gain was more than 50%.

So, just to be clear, #21: date-1358790-21.patch is the preferred patch now, right?

Nice job! I'm going to commit this to get it out into the wild just in case any problems come up, but I couldn't see that anything was broken by this change.

Thanks!

Hmmm. This really really slows down the cache creation. My page load times after clearing the cache have gone up by 30% to 50% and I'm even getting timeouts on some pages. Running tests made my hard drive go into overdrive. After the cache is populated it gets marginally better on normal pages and progressively better on pages that have lots of dates (like calendar).

Maybe there is nothing else we can do, but re-opening this to consider what, if anything, we can do about the problems of filling the cache.

And to extend on the above issue, I'm testing this on a site that has lots of date fields and date nodes and trying to load the home page, which has a view of recent date nodes, a calendar block, and an upcoming events block. I'm also testing a calendar with lots of repeating dates. Both the home page and the calendar take significantly longer to load than before when the cache is cleared.

That didn't make much of a difference, but is probably a good addition anyway. I was trying to figure out why my page load times increased so much. In my case, I have a huge number of repeating dates that each have a huge number of individual dates on them. In the older code, I was not processing all those dates, only the ones I was viewing. In the new code I am processing all of them. That is several orders of magnitude more date objects than I had before.

That is the most likely scenario I can think of where this new code will have a negative impact. Not sure if there is any way to do anything about it or not.

The more I think about this the more I think this is something to address with this patch. The patch should work fine for single value fields, or probably those with just a couple values. But for repeating date fields, or any multi-value date field with a lot of values, we are now processing every single value, even if some of them will never be viewed (or at least will probably not be viewed before the cache is cleared again). This could be a huge issue if someone has created fields that have dates that go backward or forward into years that probably won't ever be viewed by anyone.

So I wonder if we need a hybrid process. Store the date object for the first value (or perhaps a small number of multiple value fields). That will work fine for the most common cases, which are probably having one value per field. Then we need to add back some logic in the formatter for the other values to lazy load them only if they are being viewed.

I know there are sites that have date fields with dozens or maybe even hundreds of values (I hear about these things). So we need to factor that possibility into whatever we do. If nothing else, maybe others could test that kind of scenario -- create some fields that have a huge number of values and test how quickly a calendar page renders after a cache clear with and without the changes.

The other option is to just warn people if they have a lot of values in their date fields they will want to implement some sort of process to warm the cache after it is cleared. And that they may have to step up memory to handle the load when the cache is re-filled.

Thoughts?

This belongs in another feature request issue, but I will say it here anyway:

I have wondered if there might be a way to not generate each repeated date value. Rather, ONLY store the date field value & the repeat rule. Then, Drupal would query against the repeat rule to show repeated events.
That would probably be immensely complex, and certainly not something we want to tackle here and now... just a thought of mine.

Hmm, that seems like an odd place too, I would never think of looking there. Maybe we should just create a configuration form for date settings so they are all in one place? We have one for Date Popup, and Date Tools has configuration, maybe we need a general 'Date' section in admin/config form that all the Date submodules can use so all Date settings are together? We could then move the Date Popup and Date Tools configuration into that form too.

I've been thinking for a while about setting up a section in the Administration menu for Date-related items, so I made a separate issue for that and am committing that change. Then we can put this variable setting in there too.

#1397126: Add Date API section to administration menu

What if this setting would be added under the already existing Development / Performance page?
admin/config/development/performance

There is some logic to adding it to the performance page, but I'm concerned that we have Date settings scattered all over the place. Wouldn't it be easier to give people one place to go to see all the settings that Date is controlling?

It seems to me that this could be a per field setting. So the best place may be in the field settings.

+1 for #45

As challenged by tim.plunkett here is a patch with my ideas.

• Settings are in the date field settings.
• Instead of a select list for cached values provide an input box.
• Add a checkbox to enable the cache.

Seems there are some extra kittens killed in this patch. 1 by me and 1 by my ide. I can see about stripping those out if you'd like.

Ok fixed up and also fixed the test exceptions.

Will get this up here. Still needs test.

We need to get this finished and committed ASAP because we are in an unstable spot right now for anyone with a lot of repeating dates on the same field. If we can't get this done very soon I'll have to roll back the initial patch until this gets worked out.

If we make this a per-field setting we also have to have an update hook to set a value on existing dates. I think the default should be 3 or 4, not 0, or at least make that the default on repeating dates. Effectively, without this, all existing sites will suddenly start processing all dates, which could be a fatal problem for some.

The field setting itself needs an initial default value, set the same way.

Making this a field setting makes it mandatory to set a value for existing sites in an update hook. The update hook also needs to clear the field cache so the new field setting gets added to avoid missing index errors.

We need to be sure that sites that haven't yet re-loaded the cache don't get errors, since they won't have the date objects yet. Even with the right update hook we need to be sure that a site that has, for instance, a calendar block on every page, isn't going to immediately start spitting errors. So we need to swap out the code in a site with existing values and date views on every page and confirm that nothing fatal happens before update.php is run. Because I know from experience that people will swap this new code in on live sites without running the update :(

Making this a site-wide value instead a per-field setting would simplify the patch and the update process.

Before patching
--------------
- Made a date field with 6 dates.
- Made an article an filled in the dates.

After patching
--------------
- Didn't clear the cache.
- Checked the nodes ==> no problems
- Checked the date field settings, I could turn on the caching manually without a problem
- Made a new date field ==> works, no problem.

So this patch just need an update and it should be ok :)

There also probably should be a warning saying enabling this for fields with many repeating dates may degrade performance.

Hoping someone will add the tests so we can get a release out. And I like #58, it would be good to get that in.

I cleaned up the language for that setting to try to clarify some of the things that have been noted in this issue.

We still need tests. Any takers?

I'm going to do rc2 without tests. But I'm not sure what tests we need. What exactly do we want to test? That the cached object has the right value after being retrieved from the cache? That performance is improved? I'm not sure.

Referred to this issue by arlinsandbulte from my support request #1449710: "Cache dates" details: when to cache, when not to?. Reading through this, it appears the date caching would really only make a difference for repeated dates, but I may be misunderstanding. Is it worth turning on for simple non-repeating date fields? The description, "Date objects can be created and cached as date fields are loaded rather than when they are displayed to improve performance." is not all that clear to me (as someone who hadn't paid attention to the work described in this issue).

It is for multiple date fields of any kind. If you have single date fields it has no effect.

I wonder. If it's only Full Calendar that needs this, could Full Calendar not hook_form_alter() it in?

Also came here via #1449710: "Cache dates" details: when to cache, when not to? trying to figure out how on earth to explain what this field does to mere mortals. :)

It is potentially useful for any multiple value date fields. Whether it helps or not depends on what you have and what you are displaying. Here's what it does, in a nutshell:

What Tim discovered when he was having performance issues is that the process of creating a date object was time-consuming. So he was trying to find a way to reduce that.

The old code creates date objects for each date at the time it is displayed. If you have a multiple value date field with a lot of values, date objects are created only for the ones that are displayed at the time they are displayed. If most of them are not displayed that will be fine. If the same ones are displayed over and over (like on a calendar) they are re-created each time.

The new code, with caching turned on, creates date objects for every possible value at the time they are loaded, whether or not they are displayed. If they are displayed over and over they are not re-created each time. But if you have a lot of values that are never displayed, you are now creating date objects for them when they wouldn't have been created before. So for a repeating date field with a lot of values that will never be displayed at the same time, like one that extends over many years, the new option actually hurts performance.

So it's not black and white, it depends.

Forgot to add that there is a choice of how many values to cache that defaults to 4. That is because if there are lots of values on a date field, most of the time you are in the second territory -- you have a lot of values that are not going to be displayed all at the same time. So we put a limit on the number that are cached to avoid that. Anything that is not cached is created when displayed -- the old behavior.

So basically if you're concerned about performance, it's something you can try. Test the length of the page load for various views and on the node view itself to see which option seems to work the best in your situation.

Also Calendar and Full Calendar both can benefit from this, but so could a page created using Panels that has a lot of dates on it, or potentially other kinds of views of dates. So it seems to belong here rather than being hook_altered in by some other module.

It looks like this code is in the most recent dev version of calendar, should this be marked as fixed?

Yup I meant date.

I guess I'm a little confused, it seems like this patch still needs some testing done however it also seems like some bits of the caching code are already in dev... aren't those supposed to be mutually exclusive or am I missing something?

Came here via #1449710: "Cache dates" details: when to cache, when not to?, after being confused by the following text that attempts to explain the "Cache dates" checkbox when creating a date field:

Date objects can be created and cached as date fields are loaded rather than when they are displayed to improve performance.

This text is in both 7.x-2.5 and the -dev version. Until you understand the issue, on second and third reading this text still appears to be grammatically incorrect—it seems like it is either missing punctuation, or entire phrases, and is non-interpretatble. From it I got the gist that there was caching involved, but of course this was already clear from the checkbox label, "Cache dates". Noting that even webchick visited here because she couldn't understand the text (or at least, could not de-deify it), I think it needs some improvement.

Having read a good amount of the thread above, I think something like this is what the text should say:

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.

Does this correctly understand the issue, and make the text clear?

If #72 is correct, I agree that it is much, much clearer than the current text. Are there plans to change the description text on this field?

^ Yes, please. I had no clue what the current text meant. Thanks, babbage.

To clarify, this does not affect the performance of dates on node forms?

For example, 120 total single date_popup fields and 10 field collections sets, each with two additional single date fields (~10 entries each)? And the same field is never reused, other than when field collections repeat?

Would cache_form et al be caching this already?

Exactly as #72, I arrived at this issue because I was trying to understand the help text. +1 for correcting and updating the text.

Just another traveller trying to decode what this setting means. +1 for #72.

I'm a little surprised it hasn't made it into the module by now. Assuming it's due to resources and D8.