Improve queuing performance by implementing a batch process

This is the first time I hear of the rebuild process failing due to memory exhaustion. I believe the solution to this problem will be to implement a batch process for the queuing, similar to how the generation process is implemented.

Regarding your first workaround: This is by design - it's called 'rebuild queue' because it rebuilds the queue. Most of the time this is what you want - rebuilding the queue for all or specific variants and generating the sitemap. If you want to queue elements on top of queued elements, you will need to utilize \Drupal\simple_sitemap\Simplesitemap::queue instead of \Drupal\simple_sitemap\Simplesitemap::rebuildQueue The former however is not wrapped into a drush command yet (because there was no use case until yours).

Feel free to create a feature request to add that drush command, until then you can use drush php-eval "\Drupal::service('simple_sitemap.generator')->setVariants(['default', 'test'])->queue();"

Hi @gbyte,

I think you're right, the rebuild process should be batched.

However I disagree with this being treated as a feature request.

Unless it's clearly stated this module is designed with limitations around a specific number of items, this should be considered a bug.

Looking at the implementation for the rebuild process, there is a clear cap given it loads everything into memory before pushing to the queue for later generation (correct me if I'm wrong).

In regards to the rebuild queue I think that makes sense, but I would suggest (this would be a feature request) a total counter for indexed items exists in the UI. The current one can become confusing if per variant rebuilds are done and while now that you've clarified things it makes perfect sense, I was personally confused to see that counter constantly change with per variant rebuilds.

Thanks again for the great module and responsive support!

Thanks for the patch.

However, rebuilding the queue on cron won't use batch as per cron limitations. This means that for cases like ours, where the site has big amount of variants with lots of URLs the "Regenerate the sitemaps during cron runs" won't work.

That's why we might need an approach similar to what URL generation does - a queue that will be used by cron and batch:

• Create a new type of queue that will be populated in one go at the very beginning with variants to be queued (1 queue item = 1 variant)
• Process that queue: Each queued variant from the new queue will queue all its elements in the old sitemap queue
• After the new queue has been processed (and all entities queued in the old queue for generation), start that old queue.

That queue can be used by cron as well as by the batch process. This solution is not too trivial though, as parts of the UI will have to be updated as well as some logic: Right now a regenerating sitemap is defined as one where there are still queued elements to be processed. After implementing the above, should also be defined as regenerating, if the first queue is processed that queues each variant.

If you feel you want to tackle this, go ahead; I don't think I will be tackling this until there is a plan for 4.x.

Even a single variant can be huge: imagine millions of nodes of the same bundle and low amount of memory. I face "memory exhausted" problem with 1 million nodes and 256MB of memory. The method which calculates data sets (getDataSets) should be able to return chunks, not the whole data set array. It is possible to pass $chunk_no parameter to it, but this will break BC, so it's better to add new methods:
supportsChunkedDataSets()
getDataSetsChunk($chunk_no)

QueueWorker::queue() will check whether URL generator supports chunks and then call either getDataSets() as before or getDataSetsChunk() until it returns empty set.

Something like this can be used to get data sets in chunks.

Please create a merge request against the newest dev and let's colaborate on this using the new tools.

https://git.drupalcode.org/project/simple_sitemap/-/merge_requests/2/diffs

Hi all,

I wasn't able to make @maximpodorov patch work, in the sense that rebuilding the queue still ends up in an OOM.

@carolpettirossi approach works fine however it has the issue of the counter. With this approach the counter constantly increments as rebuildQueue is never specifically called. I think I can solve this by calling in my code (the one that prepares all the variants to be rebuilt) the `deleteQueue` method. This method sets `simple_sitemap.queue_items_initial_amount` to zero, which is necessary as the subsequent `queue` commends will add items to that counter.

The process then is basically like this:

1. I have a cron job that checks if sitemaps are being generated. If generation is mid way, then it exits, if it's not, I delete the sitemap queue (as the new one will be different with different variants), collect a list of variants that have expired (i.e older than my interval period) and put that in a custom queue called `rebuild_sitemap_queue`
2. I have a queue worker that goes through the `rebuild_sitemap_queue` and through the sitemap generator calls the queue method for that variant (adding the queue items to the counter.
3. A cron hook is responsible for processing the queue by calling `generateSitemap` method

I'm still running some tests but this seems to run just fine in a very large scale platform that powers hundreds of sites with several millions of records.

The only changes to the sitemap module codebase are the ones provided in @carolpettirossi's patch.

Obviously this requires you to use this module more as an API which I think is both advised and showcases the powerful nature of this module (congrats again on this @gbyte, really solid work).

I see there are some conflicts with the new release so I'll need to roll a new patch once I've tested this more solidly.

Thoughts and comments most welcome.

Cheers

Also I should add I didn't test the batch UI process in that patch.

In fact I think that should probably be a separate patch, the other changes are more critical in allowing developers to leverage the simplesitemap API in order to get something like the above done for large scale sites (and I don't see any major downsides).

This could allow you to tackle the rebuilding process itself in a separate issue for 4.x without tackling it now.

Effectively without this this would mean the only thing we change is moving this code

    if (!$this->generationInProgress()) {
      $this->rebuildQueue();
    }

out of generateSitemap in the QueueWorker (which not ideal due to the reasons mentioned above and move it inside generateSitemap in Simplesitemap.php (exactly as the existing patch) so it's a very small change, easy to review and should have minimal impact.

Let me know what you think!

Submitted Merge request and adding patch here for those that need it.

Basically I'm just moving out that call as explained above. The end result is the same but less coupled, which allows for greater flexibility.

Sorry please ignore the previous patch.

Gitlab is confusing.. When I created a fork branch from this issue, for some reason it missed a bunch of commits and so the diff in the patch above is completely incorrect. The merge request should still be fine.

Nevertheless here is a patch produced the old school way.

Adding the patch here for the patch queue rebuild only.

This allows the rebuild of the queue via batch in both UI and drush and fixes an issue on comment #5 where the variants being provided were being ignored.

Just to provide a final summary:

1. Merge request 17 (or patch provided in #18) decoupled the rebuild queue call, moving it outside the QueueWorker.
2. Merge request 18 (or patch provided in #23) provide batch support for the rebuild process on both UI and Drush

This is not enough, see #7. Even a single variant can be too huge to fit in memory.

Should we combine the batch process with the chunked data to mitigate all scenarios? The path you provided isn't enough either. With a lot f variants, we still go OOM.

Updating the patch to pass the QueueWorker GENERATE_TYPE_FORM to the downstream method rebuildQueue()

Even with 3 patches #8, #23, #27 applied via composer-patches issue still occurs:

>  [notice] 858 out of 335272 total queue items have been processed.
>  [notice] Memory: 215.81 MB, non-peak mem: 200.5 MB
>  [notice] 859 out of 335272 total queue items have been processed.
>  [notice] Memory: 219.81 MB, non-peak mem: 204.5 MB
>  [notice] 860 out of 335272 total queue items have been processed.
>  [notice] Memory: 219.81 MB, non-peak mem: 204.5 MB
>  [notice] 863 out of 335272 total queue items have been processed.
>  [notice] Memory: 226.01 MB, non-peak mem: 208.5 MB
>  [notice] 866 out of 335272 total queue items have been processed.
>  [notice] Memory: 227.88 MB, non-peak mem: 208.5 MB
...
>  [notice] 883 out of 335272 total queue items have been processed.
>  [notice] Memory: 250.52 MB, non-peak mem: 216.5 MB
> PHP Fatal error:  Allowed memory size of 268435456 bytes exhausted (tried to allocate 7293057 bytes) in docroot/core/lib/Drupal/Core/Database/StatementWrapper.php on line 116
> PHP Stack trace:

What is strange is that fact so we have increasing memory consumption after each batch: 215 => 219 => 226 => 227 => 250 until 256M is reached.

Seems that we need to unset some variables to free the memory or functionality should be refactored e.g. to use generators instead of loading whole array into memory.

Consumption of memory just by bootstrap and drush:

drush st -vvv
 [preflight] Redispatch to site-local Drush: '/vendor/drush/drush/drush'.
...
 [debug] Starting bootstrap to max [0.26 sec, 9.47 MB]
...
 [debug] Finished bootstrap of the Drupal Kernel. [0.51 sec, 22.12 MB]
...
 [debug] Add a commandfile class: Drupal\pathauto\Commands\PathautoCommands [0.91 sec, 35.8 MB]
 Drupal version   : 9.2.4
...

upd:
Created separate issues as issue that I've found is not related to variant chunks:

• https://www.drupal.org/project/simple_sitemap/issues/3231298
• https://www.drupal.org/project/simple_sitemap/issues/3231308

3.x is at feature freeze and 4.x has been deemed good enough for non-api-dependant use cases. Let's test the necessity of this in 4.x and implement if necessary.

Re #7:
This is the issue we ran into (we have more than 5 million nodes after a D7 migration).

In our case, even the query cannot be executed because the statement itself consumes all the world's memory.