Problem/Motivation

The batch operation queries all nodes and then creates an operation for every single ID. That creates a massive batch array that has be stored and loaded on every batch step, it might also time out or run into memory issues.

Steps to reproduce

A better approach would to use the batch sandbox, calculate the count, and then do a query to get N ids, create queue items for them and then increment the index. Possibly something like sort by nid, store the largest nid and then proceed with that on the next query.

Proposed resolution

  1. No longer looks up queue items by querying the database queue allowing the module to be used with other queue types.
  2. Use the same batch for Drush and the UI
  3. Changing the batch operation to process each node type repeatedly until all nodes are in the queue.

Remaining tasks

User interface changes

None

API changes

  • \Drupal\node_revision_delete\NodeRevisionDeleteInterface::nodeExistsInQueue() returns a bool depending on whether the node is in the table and not an int with the queue ID.
  • All queue items should be created via the new method \Drupal\node_revision_delete\NodeRevisionDeleteInterface::createQueueItem()
  • The new method \Drupal\node_revision_delete\NodeRevisionDeleteInterface::removeNodeFromQueueMap() deletes a node from new map table
  • \Drupal\node_revision_delete\NodeRevisionDeleteInterface::deleteItemFromQueue() has been removed - it is no longer necessary
  • \Drupal\node_revision_delete\NodeRevisionDeleteBatchInterface::queueBatch() optionally takes a list of NodeType objects
  • \Drupal\node_revision_delete\NodeRevisionDeleteBatchInterface::queue() has been changed to not take an $nid argument
  • Removed the hook implementation node_revision_delete_node_delete()

Data model changes

New table to determine if a node is in the queue.

Issue fork node_revision_delete-3508327

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

berdir created an issue. See original summary.

adriancid’s picture

adriancid
Primary language Spanish
Location Qc, Canada 🇨🇦
commented

Hi @berdir, thanks for this issue, I'm not having much time these days and I see you're very active in the issue queue, do you want to become a maintainer?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I was actually considering that recently, but then saw that you pushed a new release. I did a bit of a review and testing around updating 1.x to 2.x and just wanted to write down my findings.

Feel free to add me, so there's another person around if a release is necessary or so, but I can't really promise much beyond that. I might or might not work on this issue (and others), depending on how much it's a problem for us, haven't yet tested where the limits are.

onnia made their first commit to this issue’s fork.

onnia opened merge request !34

onnia’s picture

onnia commented

Hi,
My merge request does two things: The nodeExistsInQueue check is replaced with array of all node ids and a isset check, this method cuts the run time from tens of minutes to seconds. Second thing is the batch creation uses chunks of node ids that are processed. The chuck size can be set via the queue_chunk_size config. I also looked into updating drush queue adding with 587000 nodes, the drush (time drush node-revision-delete:queue) adding takes 2 mins. I still have to commit my fix for the drush command.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

What's the memory usage with that amount of content? drush -vvv should report that.

My idea was something like https://git.drupalcode.org/project/tmgmt/-/blob/8.x-1.x/sources/content/..., with a sandbox and a query that progresses through it. but that would of course be a lot slower.

It's a bit awkward that the batch methods are on an interface, that technically makes this an API break, my method would too. Seems like something like that should be an internal implementation, but tricky to step back from that now, the module is stable.

onnia’s picture

onnia commented

I did some memory debugging with this helper: https://blog.riff.org/2016_08_04_how_to_display_time_and_memory_use_for_drush_commands
When the 587k node ids are already in the queue and the $nids_in_queue is at largest 

Memory:
  Initial: malloc = 21.60M           real = 22.25M
  Final:   malloc = 22.58M (+ 0.99M) real = 49.25M (+27.00M)
  Peak:    malloc = 80.03M (+58.43M) real = 96.25M (+74.00M)

Yes, the memory usage peaks. About the api changes: I did not consider the breaking changes when editing the BatchInterface.

onnia’s picture

onnia commented

If the edits to the src/NodeRevisionDeleteBatchInterface.php are an issue, then the MR could be altered to create a custom method for the batch queue which processes the $nids_chunk. This new queueChunk($nids_chunk) method could be used when the count of processed nids_to_add gets larger then eg 1000? The new method could be added here -> https://git.drupalcode.org/project/node_revision_delete/-/merge_requests...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Category: Task » Bug report

This is totally a problem but I think we should have a solution that works without configuration.

There are three kinda related problems here. The first problem is the way the batch is created 1 row per node which is what this issue has been solving. The second problem is the use of a query to identify nodes already in the queue. It's doing a look on a non-indexed field and relying on the queue backend to be the Drupal database. And thirdly, the whole way of queuing when a node revision is created only works node_revision_delete plugins which are based on revision amounts not time.

Given this particular issue is about scaling I propose with solve 1 and 2 here. Also I feel this is a bug for this module given the wish to remove revisions often comes when you hit limits due to having a big site.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

We've been evaluating this module and faced with this issue when installing and queueing all the items for the first time.

For 1st item mentioned in last comment, agree that current approach, or berdir's in #7 would be very helpful.
For 2nd item, I created #3564120: Make this module dependent on Queue Unique that might imply some rearchitecture, but I think it might be worth
For 3rd item, created a MR in #3512434: Outdated revisions are not added to queue unless they are resaved that tries to address the expired revisions in unmodified nodes issues.

What do you think?

alexpott opened merge request !57

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review

alexpott changed the visibility of the branch 3508327-batch-to-add to hidden.

alexpott changed the visibility of the branch node_revision_delete-3508327-3508327-batch-to-add to hidden.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I've tested this on a site with 10000 nodes and the queue is added twice as quick. Also drush and the UI now use the same batch so have the same performance characteristics. I think the interface changes here are acceptable for a minor release because in reality the node_revision_delete service and node_revision_delete.batch: service should not be APIs - the API of this module is the plugins changing these services would be very fragile.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

locking is fairly expensive,

Compared to deleting a node revision? Or even 50 node revisions? I don't think that that can be true. @berdir why do you think it is expensive? Contention?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Actually I have a concern, doing the insert into the queue and the map in a transaction as we will be will cause deadlocks. Need to move that to a post transaction callback.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> Compared to deleting a node revision? Or even 50 node revisions? I don't think that that can be true. @berdir why do you think it is expensive? Contention?

Well, my understanding is that you add all nodes to the queue, so maybe 9000 or 9500 of your 10k will actually not have anything to delete, so they get the queue item, lock it, figure out there's nothing to do, and then free the lock again, right?

Anyway, you're probably right, I didn't profile it, even with that, the queue itself also reads and updates and deletes queue items in the database, and this is likely not a significant cost.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community

I've implemented #18

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community

I changed approach and removed the lock. I've changed the map table into our own semaphore thereby making the lock unnecessary.

  • alexpott committed 3e3e6786 on 2.1.x 
    perf: #3508327 Batch to add all nodes to queue loads all nodes at once...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.