Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.The patch committed in #2851552: On rebuild, xmlsitemap_node fills node static cache, exhausts memory has nasty side-effects:
--- a/xmlsitemap_node/xmlsitemap_node.module
+++ b/xmlsitemap_node/xmlsitemap_node.module
@@ -42,7 +42,7 @@ function xmlsitemap_node_xmlsitemap_index_links($limit) {
* An array of node IDs.
*/
function xmlsitemap_node_xmlsitemap_process_node_links(array $nids) {
- $nodes = node_load_multiple($nids);
+ $nodes = node_load_multiple($nids, array(), TRUE);
foreach ($nodes as $node) {
$link = xmlsitemap_node_create_link($node);
xmlsitemap_link_save($link, array($link['type'] => $node));
Resetting an entity_load cache bin in Drupal 7 can have more drastic consequences than in previous versions of Drupal, as we can have persistent load caches in place of the PHP static memory cache.
Instead, wherever possible, only flush the necessary entities from the cache, using entity_get_controller($entity_type)->resetCache(array($entity_id));
The previously-committed patch purges the entire node cache for no reason. That patch should be reverted and replaced by code which flushes only the specific nodes which are being processed. That deals with the memory concerns without touching the cached nodes which are not in the $nids array.
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | xmlsitemap-entity_load_cache_reset-2986733-22.patch | 1.14 KB | pifagor |
| |||
Comments
Comment #2
jweowu CreditAttribution: jweowu at Catalyst IT commentedComment #3
jweowu CreditAttribution: jweowu at Catalyst IT commentedAlso see #1963964: Don't use $reset flag with node_load() which I posted a patch for 5 years ago, and which remains uncommitted :(
Please don't ever commit code which passes the $reset flag to node_load(). If you ever see that in a proposed patch you should assume it's a bug, unless you can establish that flushing specific cache entries is somehow insufficient.
Comment #4
jweowu CreditAttribution: jweowu at Catalyst IT commentedComment #5
jweowu CreditAttribution: jweowu at Catalyst IT commentedComment #6
pifagorI think we should not change
$nodes = node_load_multiple($nids, array(), TRUE);toforeach ($nids as $nid)$node = node_load($nid);Comment #7
jweowu CreditAttribution: jweowu at Catalyst IT commentedAs I've commented on in #2851552: On rebuild, xmlsitemap_node fills node static cache, exhausts memory regarding the original commit:
That is why my patch avoids that.
You could refactor to
node_load_multiple()in batches of a fixed/safe size (feel free to do that; I felt it was an unnecessary complication, but I don't object to it), but it's not safe to assume you can load any number of nodes -- after all, that's precisely what was causing somebody memory issues in the first place.Edit: Not to mention that your suggested revision restores the code that purges the entire node load cache, which is the precise thing I'm trying to eliminate here. That
TRUEin the arguments tonode_load_multiple()is a massive fail, and should never be used.Comment #8
jweowu CreditAttribution: jweowu at Catalyst IT commentedComment #9
pifagorNo, look please patch https://www.drupal.org/files/issues/2018-07-26/xmlsitemap-entity_load_ca...
entity_get_controller('node')->resetCache(array($node->nid));Comment #10
jweowu CreditAttribution: jweowu at Catalyst IT commentednode_load_multiple($nids, array(), TRUE);purges the entire node cache. You absolutely cannot keep that.Comment #11
jweowu CreditAttribution: jweowu at Catalyst IT commentedYou could split
$nidsinto batches of a known size, and usenode_load_multiple($batch)if you really wish to call node_load_multiple. You mustn't use $reset though.Comment #12
pifagorOf course, I agree with you, because it was worth changing the code line on
$nodes = node_load_multiple($nids, array(), FALSE);In order to optimize my work, I will review your decision again.
Thanks for the info.
Comment #13
pifagorNew patch
Comment #14
jweowu CreditAttribution: jweowu at Catalyst IT commentedFWIW I'm not a big fan of the variable names.
$new_nidsis neither "new" (they're the same node IDs as$nids), nor does it directly contain "nids" (being an array of arrays of nids); and$valueis almost always a meaningless name.Something like
foreach ($nids_chunks as $chunk)would be more meaningful in context.I'm utterly perplexed by
count($nids)/(count($nids)/15). That's an extraordinarily elaborate way of writing15(not to mention that you're dividing by zero if $nids is empty, but that's a bit of a side-issue in this instance).node_load_multiple($value, array());should just benode_load_multiple($value);(ornode_load_multiple($chunk);if you were to use my suggested name).And as you're using chunks, you could move
entity_get_controller('node')->resetCache(array($node->nid));outside of the loop, and change it toentity_get_controller('node')->resetCache($chunk);Comment #15
pifagorComment #16
pifagorComment #18
pifagorComment #19
jweowu CreditAttribution: jweowu at Catalyst IT commentedcount($nids)/(count($nids)/15)is crazy code. I don't know how you came up with that, or what you think it's doing, but aside from the division by zero, it's literally the same thing as15. Do the algebra:(A / (A/B)) == (BA / A) == BIt's just an inexplicably convoluted way of writing
15. I said as much in my previous reply.Remove the new
if (count($nids) >= 1)test -- you don't want to protect the crazy code from dividing by zero; you want to remove the crazy code entirely, by changingcount($nids)/(count($nids)/15)to15.Comment #20
pifagorI'm sorry that I missed this remark. I fixed the patch.
Comment #21
jweowu CreditAttribution: jweowu at Catalyst IT commentedPersonally I would still ditch the
if (count($nids) >= 1)test, as it doesn't serve much purpose -- if$nidsis empty then$nids_chunkswill also be an empty array, and the foreach loop around that empty array would do nothing; so I don't think it's important to be testingcount($nids). I'm not particularly bothered by it though.I'd change the comment
// Load no more than 15 nodes.to// Load no more than 15 nodes at a time., as the current wording sounds a bit like all the other nodes will be completely ignored.Everything else now looks good to me.
Comment #22
pifagorDone.
This is required for automatic tests, otherwise the patch will not be tested. And this check does not hurt.
Comment #23
jweowu CreditAttribution: jweowu at Catalyst IT commented> This is required for automatic tests, otherwise the patch will not be tested.
I don't understand why that would be.
I know that you added this check in #18 after your patch in #15 failed testing; but the reason it failed testing was that you had that division-by-zero error in the crazy code (which you have removed in #20).
With the cause of the division-by-zero now gone, I'd be very surprised if removing that check caused any testing problems.
> And this check does not hurt.
Agreed. I'm just mostly sure it also doesn't help :)
I don't mind either way, though, so the patch looks good to me! (I'd mark it RTBC but I've not actually tested it.)
Comment #24
alex_optimPatch looks not bad.
Comment #25
pifagorComment #27
pifagor