Moving expired locks to the repair

[stuartskelton]

Hi,

I make high use of locks in our system to globally limit jobs, and have noticed locking can get slow if you are using them in anger.

I have experimented with moving the lock removal to the repair function, and have seen some improvements.

using the following change to the benchmark script

my $LOCK        = 100_000;

# Lock
say "Acquiring locks $LOCK times";
$before = time;
$minion->lock($_ % 2 ? 'foo' : 'bar', 1, {limit => int($LOCK / 2)}) for 1 .. $LOCK;
$elapsed = time - $before;
$avg     = sprintf '%.3f', $LOCK / $elapsed;
say "Acquired locks $LOCK times in $elapsed seconds ($avg/s)";

sleep 5;
say "Acquiring locks $LOCK times";
$before = time;
$minion->lock($_ % 2 ? 'foo' : 'bar', 1, {limit => int($LOCK / 2)})for 1 .. $LOCK;
$elapsed = time - $before;
$avg     = sprintf '%.3f', $LOCK / $elapsed;
say "Acquired locks $LOCK times in $elapsed seconds ($avg/s)";

If I run this against Minion 11.0

#1st run
% perl -Mlocal::lib=local -Ilib  examples/minion_bench.pl
Acquiring locks 100000 times
Acquired locks 100000 times in 269.404798984528 seconds (371.189/s)
Acquiring locks 100000 times
Acquired locks 100000 times in 300.946390151978 seconds (332.285/s)
#2nd run
% perl -Mlocal::lib=local -Ilib  examples/minion_bench.pl
Acquiring locks 100000 times
Acquired locks 100000 times in 203.707815885544 seconds (490.899/s)
Acquiring locks 100000 times
Acquired locks 100000 times in 228.127815961838 seconds (438.351/s)

With the following diff

diff --git a/lib/Minion/Backend/Pg.pm b/lib/Minion/Backend/Pg.pm
index 21a4d30..d2af7e4 100644
--- a/lib/Minion/Backend/Pg.pm
+++ b/lib/Minion/Backend/Pg.pm
@@ -202,6 +202,9 @@ sub repair {
     q{UPDATE minion_jobs SET state = 'failed', result = '"Job appears stuck in queue"'
       WHERE state = 'inactive' AND delayed + ? * INTERVAL '1 second' < NOW()}, $minion->stuck_after
   );
+
+  # Expired locks
+  $db->query("DELETE FROM minion_locks WHERE expires < NOW()");
 }

 sub reset {
diff --git a/lib/Minion/Backend/resources/migrations/pg.sql b/lib/Minion/Backend/resources/migrations/pg.sql
index e718f00..b51988a 100644
--- a/lib/Minion/Backend/resources/migrations/pg.sql
+++ b/lib/Minion/Backend/resources/migrations/pg.sql
@@ -95,3 +95,20 @@ ALTER TABLE minion_jobs ADD COLUMN lax BOOL NOT NULL DEFAULT FALSE;

 -- 24 up
 CREATE INDEX ON minion_jobs (finished, state);
+
+-- 25 up
+-- Update minion_lock function to remove cleanup (now handled by repair())
+CREATE OR REPLACE FUNCTION minion_lock(TEXT, INT, INT) RETURNS BOOL AS $$
+DECLARE
+  new_expires TIMESTAMP WITH TIME ZONE = NOW() + (INTERVAL '1 second' * $2);
+BEGIN
+  lock TABLE minion_locks IN exclusive mode;
+  IF (SELECT COUNT(*) >= $3 FROM minion_locks WHERE NAME = $1 AND expires > NOW()) THEN
+    RETURN false;
+  END IF;
+  IF new_expires > NOW() THEN
+    INSERT INTO minion_locks (name, expires) VALUES ($1, new_expires);
+  END IF;
+  RETURN TRUE;
+END;
+$$ LANGUAGE plpgsql;

I get:

#1st run
% perl -Mlocal::lib=local -Ilib  examples/minion_bench.pl
Acquiring locks 100000 times
Acquired locks 100000 times in 38.5793731212616 seconds (2592.059/s)
Acquiring locks 100000 times
Acquired locks 100000 times in 39.7984459400177 seconds (2512.661/s)
#2nd run
perl -Mlocal::lib=local -Ilib  examples/minion_bench.pl
Acquiring locks 100000 times
Acquired locks 100000 times in 39.7343950271606 seconds (2516.711/s)
Acquiring locks 100000 times
Acquired locks 100000 times in 39.6178710460663 seconds (2524.113/s)
Releasing locks 100000 times
Releasing locks after wait 100000 times in 14.7822351455688 seconds (6764.877/s)

with the default on of 1000 locks,

11.0

% perl -Mlocal::lib=local -Ilib  examples/minion_bench.pl
Acquiring locks 1000 times
Acquired locks 1000 times in 0.308051109313965 seconds (3246.215/s)
Acquiring locks 1000 times
Acquired locks 1000 times in 0.304666042327881 seconds (3282.282/s)
Releasing locks 1000 times
Releasing locks after wait 1000 times in 0.158185005187988 seconds (6321.712/s)

with patch

% perl -Mlocal::lib=local -Ilib  examples/minion_bench.pl
Acquiring locks 1000 times
Acquired locks 1000 times in 0.263604164123535 seconds (3793.567/s)
Acquiring locks 1000 times
Acquired locks 1000 times in 0.242500066757202 seconds (4123.710/s)
Releasing locks 1000 times
Releasing locks after wait 1000 times in 0.163039922714233 seconds (6133.467/s)

The above tests have very sort expiries, but there is still a uplift if there are no expiring locks within the testing timeframe (eg an expiry time of 3600)

11.0

perl -Mlocal::lib=local -Ilib  examples/minion_bench.pl
Acquiring locks 100000 times
Acquired locks 100000 times in 587.617259025574 seconds (170.179/s)
Acquiring locks 100000 times
Acquired locks 100000 times in 1151.04746508598 seconds (86.877/s)
Releasing locks 100000 times
Releasing locks after wait 100000 times in 23.8147079944611 seconds (4199.086/s)

Patched

perl -Mlocal::lib=local -Ilib  examples/minion_bench.pl
Acquiring locks 100000 times
Acquired locks 100000 times in 314.749639987946 seconds (317.713/s)
Acquiring locks 100000 times
Acquired locks 100000 times in 592.630260944366 seconds (168.739/s)
Releasing locks 100000 times
Releasing locks after wait 100000 times in 21.1692979335785 seconds (4723.822/s)

If you think this is worth it I can get this into a pull request, or want some more testing I do that.

[kraih]

That does look interesting. Wonder if there are any downsides.

[stuartskelton]

There is a mental model change as it breaks the idea that locks should look after themselves, and relies on users not messing to much with the repair timeout.

That said, Dealing the locks table, the queries seem to be covered by an index.

The two exceptions I see are:

1. deleting, as expiry is only used
2. when using stats, this also only uses expiry

so there is a table scan. Adding an index (CREATE INDEX ON minion_locks (expires)) to stock 11.0 does speed things up however obviously not as much as just not doing it.

That said adding the index could be useful for the areas above.