Stop flushing entire entity load cache bins unnecessarily

Re-roll against current HEAD

A note about the order of events (for the cases where a $reset argument has been removed from an entity load call).

The old code is resetting the load cache before loading (& saving) the node, and the new code is removing the node from the cache after loading (& saving) the node, which is obviously a functional change.

I've done this because the reason for the presence of $reset=TRUE in the original node_load() calls is not that the node objects in cache are invalid and need regenerating, but simply because the code is looping over some arbitrary number of node objects and it wanted to ensure the memory usage couldn't balloon out of control (as the default entity load cache is in static memory). This was a common pattern for dealing with memory usage in such scenarios in previous versions of Drupal.

Flushing each specific node after saving it still resolves the memory usage concern, but also allows the node to be loaded quickly from cache if it was present, which may provide a performance advantage. (Obviously it will be regenerated the next time it is loaded, but this was also true -- for all but the final node processed -- with the original code.)

The two code changes which are not simply comments are in _node_mass_update_helper(), which is only called via node_mass_update(); and node_access_rebuild(). I do not believe there is any reason why either of these functions should not safely use the node objects loaded from the entity cache. (And likewise for the third instance, which appears in example comments.)

In short, no Drupal code ever has reason to distrust the entity load cache unless it specifically knows that the entity data has been directly modified (e.g. via direct database queries) but not also flushed from the load cache at that time. It is the responsibility of any code making such direct modifications to flush those entities from the cache. None of the functions in this patch are making any such direct changes to node data, and therefore they should trust that the cached nodes are valid.

I've now noticed a call to node_load_multiple() with $reset which wasn't covered. I'll audit usage, and update the patch.

On review, some of these instances are only used in batch operations, with hard-coded limits on the number of nodes which can be loaded at a time (20 being the highest).

I'm inclined to stop calling entity_get_controller($entity_type)->resetCache(array($entity_id)); for these instances (i.e. performing no cache flushing at all).

If the load cache is persistent, we never actually wanted to flush them -- we just thought that we needed to, on account of the static memory case.

However, even if the load cache is in static memory, I think we can reasonably expect that 20 nodes will not exceed memory limits in a single batch request! The Drupal 7 patch for #375494: Restrict the number of nodes held in the node_load() static cache started with a value of 20 and ended up with a value of 100 in its last version, so I feel confident about this.

I've yet to test this version myself, but here's the result of the new audit, in which I checked *load(), *load_multiple(), and resetCache() calls.

Changes are:

1. Fixed the example code in callback_batch_operation() in form.api.php (which is more recent than my original patch).

2. No longer flushing entities from the load cache during size-limited batch operations (as per my previous comment above).

3. Noting that node_save() explicitly flushes the node from the load cache, and deferring to that where appropriate.

4. Converted resetCache() calls (with no argument) to targeted resetCache($entity_ids) calls (of which discussion now follows...)

The obviously-valid call to resetCache() in taxonomy_terms_static_reset() and the familiar one in entity_load() have been left alone.

The ability to pass entity IDs to resetCache() was added on 2010-11-30, and the following two instances pre-date that change:

* node_delete_multiple(): Converted entity_get_controller('node')->resetCache() to resetCache($nids). The original code dates to 2009-10-03. Also corrected the accompanying comment, which was incorrect on account of a call to cache_clear_all() having since been removed.

* user_delete_multiple(): Converted entity_get_controller('user')->resetCache() to resetCache($uids). The original code dates to 2010-03-12.

There's a third one of these, but (a) it post-dates the aforementioned change, and (b) the patch which added it has a confusing and undocumented mixture of specific entity flushing, and entire cache bin flushing. I've requested clarification on this in #2159549: file_save() and file_delete() do not clear entity controller cache:

* file_delete(): Converted entity_get_controller('file')->resetCache() to resetCache(array($file->fid)).

I note a couple of things about all that:

Firstly, the functions which are flushing an entire cache bin by directly calling resetCache() are all functions for deleting entities. Is there some accepted reason why deleting an entity should invalidate all other cached entities of the same type? (and only of the same type). My present interpretation is that the 'node' and 'user' cases were never updated after the ability to flush specific entities was added, and that the 'file' case simply followed in the footsteps of the other two. (edit: See also https://www.drupal.org/node/221081#comment-5897484 ).

Secondly, comment_delete_multiple() does not flush any cached entities at all, which looks like a bug (edit: except the cache is disabled in comment_entity_info() by default, so not a bug after all, but potentially confusing).

...that the 'file' case simply followed in the footsteps of the other two

Dave Reid has confirmed that the cache bin flush added to file_delete() was for consistency with the existing code in node_delete_multiple() and user_delete_multiple().

I believe this to be a mistake in all cases.

Lending further weight to this, I've just tracked down EntityStorageBase::delete() in Drupal 8.0.x, and it very much looks as if this bug has been fixed in Drupal 8:

    // Perform the delete and reset the static cache for the deleted entities.
    $this->doDelete($keyed_entities);
    $this->resetCache(array_keys($keyed_entities));

No issues (backwards-compatibility or otherwise) noted in the version I've been running since 2013.

(Which was #1, and then re-rolled at #2)

When I did the most recent audit I had been hoping for some additional core developer input on the 'delete' cases discussed in #6 and #8, and consequently I've not yet been using that version of the patch, so I can't say anything more conclusive about it at present. If you would be willing to give this any testing/review, that would be hugely appreciated -- as you say, it's awfully unfortunate that this is still a core issue.

I don't think any new automated tests are needed. The existing tests all passed, and we don't need to verify how the $reset argument works.

It's sad that there was never any core maintainer input on this performance issue.

The recent activity has prompted me to review the issue and begin using #6 in my non-production environments. Thus far I've not encountered any problems with it, so I would encourage others to do likewise and report back.

I've realised that I didn't touch taxonomy_term_delete() in #6. What I did comment on was:

The obviously-valid call to resetCache() in taxonomy_terms_static_reset() [has] been left alone.

which still seems like the correct thing to do for that function as it stands; however the uses of taxonomy_terms_static_reset() seem excessive, and clearly constitute another instance of the general issue being addressed here.

taxonomy_terms_static_reset() is called by both taxonomy_term_delete() and taxonomy_term_save(), and I feel that both of those functions should be acting in a more targeted fashion.

Both of these calls date back to 2009-04-02 in commit 3a35e28f9 which, as per my research in #6, pre-dates the ability to pass IDs to resetCache() which happened on 2010-11-30, but was never retro-fitted to these usages.

The static reset function definition is:

/**
 * Clear all static cache variables for terms.
 */
function taxonomy_terms_static_reset() {
  drupal_static_reset('taxonomy_term_count_nodes');
  drupal_static_reset('taxonomy_get_tree');
  drupal_static_reset('taxonomy_get_tree:parents');
  drupal_static_reset('taxonomy_get_tree:terms');
  drupal_static_reset('taxonomy_get_parents');
  drupal_static_reset('taxonomy_get_parents_all');
  drupal_static_reset('taxonomy_get_children');
  entity_get_controller('taxonomy_term')->resetCache();
}

I'm treating the drupal_static_reset() calls as out of scope -- I'm not going to touch that behaviour in this issue. I'm only interested in the resetCache() call, and I suspect the thing to is to simply add an optional $ids argument to the function which, if provided, can be passed along. (I also note that this is exactly what was done for taxonomy_vocabulary_static_reset(), but for some reason the equivalent was not added to taxonomy_terms_static_reset().)

taxonomy_term_delete() and taxonomy_term_save() can then utilise that argument to stop purging all other terms from the entity load cache.

taxonomy_vocabulary_delete() is a similar case with its no-argument call to taxonomy_vocabulary_static_reset(), although deleting a vocabulary is such an uncommon occurrence (certainly for most sites) that this seems by far the lowest-priority.

Patch updated to deal with taxonomy terms and vocabularies.

Now marking the $reset parameter as deprecated in the various load functions, as it's unsafe to use in general, and resetCache() provides a more explicit alternative in the highly-unlikely situation that this action is genuinely required. In addition, entity_load_unchanged() is mentioned as a potential alternative in each case.

False-negative from the test bot. The interdiff from the last patch is literally just changes to comments in the code.

I'm now running #19 in my production system, following more than a year of using it in development and staging with no apparent issues.

(Prior to that production has been running the changes from #1 ever since 2013.)

You're welcome, @alesr. I recommend also auditing your custom and contrib code for any uses of the $reset argument or resetCache() without an argument. I just grep for *load(), *load_multiple(), and resetCache() calls, and eyeball them to check that they're sane. As you'll notice from the list of related issues in the sidebar, it's very easy for contrib code to purge an entity cache for no good reason, so there's always a possibility of such a bug in some module you're using (and sadly, given this core patch was never merged, there still isn't any core documentation warning contrib authors not to use those functions that way).

Edit: While I'm here, this marks approximately one year running #19 in a large production site with no issues (and more than two years in other environments), so AFAIK this patch is solid.