From 85d803b7336c39f8e7e432d6e229790cc628cefc Mon Sep 17 00:00:00 2001
From: Eric Mc Sween <eric.mcsween@overleaf.com>
Date: Fri, 18 Mar 2022 10:25:11 -0400
Subject: [PATCH] perf: speed up clean operation

The clean operation on sets backed by lists (wait, active, paused)
quickly gets very slow when the list is large. This is because each job
deletion scans the whole list in a LREM call, resulting in O(N * M)
complexity where N is the number of jobs in the list and M is the number
of jobs to delete.

With this change, the deletion is done in two passes. The first pass
sets each item that should be deleted to a special value. The second
pass deletes all items with that special value in a single LREM call.
This results in O(N) complexity.
---
 lib/commands/cleanJobsInSet-3.lua | 17 +++++++++++++++--
 test/test_queue.js                |  9 +++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/lib/commands/cleanJobsInSet-3.lua b/lib/commands/cleanJobsInSet-3.lua
index 98a98d906..b0a110191 100644
--- a/lib/commands/cleanJobsInSet-3.lua
+++ b/lib/commands/cleanJobsInSet-3.lua
@@ -48,12 +48,14 @@ local jobIds = rcall(command, setKey, rangeStart, rangeEnd)
 local deleted = {}
 local deletedCount = 0
 local jobTS
+local deletionMarker
 
 -- Run this loop:
 -- - Once, if limit is -1 or 0
 -- - As many times as needed if limit is positive
 while ((limit <= 0 or deletedCount < limit) and next(jobIds, nil) ~= nil) do
-  for _, jobId in ipairs(jobIds) do
+  local jobIdsLen = #jobIds
+  for i, jobId in ipairs(jobIds) do
     if limit > 0 and deletedCount >= limit then
       break
     end
@@ -63,7 +65,14 @@ while ((limit <= 0 or deletedCount < limit) and next(jobIds, nil) ~= nil) do
       jobTS = rcall("HGET", jobKey, "timestamp")
       if (not jobTS or jobTS < maxTimestamp) then
         if isList then
-          rcall("LREM", setKey, 0, jobId)
+          if deletionMarker == nil then
+            -- we could use anything as a deletion marker; it's convenient to
+            -- use a job id that we know we're going to delete
+            deletionMarker = jobId
+          end
+          -- replace the entry with a deletion marker; the actual deletion will
+          -- occur at the end of the script
+          rcall("LSET", setKey, rangeEnd - jobIdsLen + i, deletionMarker)
         else
           rcall("ZREM", setKey, jobId)
         end
@@ -101,4 +110,8 @@ while ((limit <= 0 or deletedCount < limit) and next(jobIds, nil) ~= nil) do
   end
 end
 
+if isList and deletionMarker ~= nil then
+  rcall("LREM", setKey, 0, deletionMarker)
+end
+
 return deleted
diff --git a/test/test_queue.js b/test/test_queue.js
index 87fc4ea48..9a9265875 100644
--- a/test/test_queue.js
+++ b/test/test_queue.js
@@ -2983,6 +2983,15 @@ describe('Queue', () => {
         });
     });
 
+    it('should succeed when the limit is higher than the actual number of jobs', async () => {
+      await queue.add({ some: 'data' });
+      await queue.add({ some: 'data' });
+      const deletedJobs = await queue.clean(0, 'wait', 100);
+      expect(deletedJobs).to.have.length(2);
+      const remainingJobsCount = await queue.count();
+      expect(remainingJobsCount).to.be.eql(0);
+    });
+
     it("should clean the number of jobs requested even if first jobs timestamp doesn't match", async () => {
       // This job shouldn't get deleted due to the 5000 grace
       await queue.add({ some: 'data' });