Refactor download tracking to use a single EVAL command to minimize redis round trips

src/Packagist/Redis/DownloadsIncr.php

@@ -4,21 +4,53 @@ namespace Packagist\Redis;
 
 class DownloadsIncr extends \Predis\Command\ScriptCommand
 {
+    private $args;
+
     public function getKeysCount()
     {
-        return 7;
+        if (!$this->args) {
+            throw new \LogicException('getKeysCount called before filterArguments');
+        }
+
+        return count($this->args);
+    }
+
+    protected function filterArguments(array $arguments)
+    {
+        $this->args = $arguments;
+
+        return parent::filterArguments($arguments);
     }
 
     public function getScript()
     {
         return <<<LUA
-redis.call("incr", KEYS[1]);
-redis.call("incr", KEYS[2]);
-redis.call("incr", KEYS[3]);
-redis.call("incr", KEYS[4]);
-redis.call("incr", KEYS[5]);
-redis.call("incr", KEYS[6]);
-redis.call("incr", KEYS[7]);
+local doIncr = false;
+local successful = 0;
+for i, key in ipairs(KEYS) do
+    if i == 1 then
+        -- nothing
+    elseif ((i - 2) % 7) == 0 then
+        local requests = redis.call("INCR", key);
+        if 1 == requests then
+            redis.call("EXPIRE", key, 86400);
+        end
+
+        doIncr = false;
+        if requests <= 10 then
+            doIncr = true;
+            successful = successful + 1;
+        end
+    elseif doIncr then
+        redis.call("INCR", key);
+    end
+end
+
+if successful > 0 then
+    redis.call("INCRBY", KEYS[1], successful);
+end
+
+return redis.status_reply("OK");
 LUA;
     }
 }

src/Packagist/WebBundle/Controller/ApiController.php

@@ -138,7 +138,7 @@ class ApiController extends Controller
             return new JsonResponse(array('status' => 'error', 'message' => 'Package not found'), 200);
         }
 
-        $this->trackDownload($result['id'], $result['vid'], $request->getClientIp());
+        $this->get('packagist.download_manager')->addDownloads(['id' => $result['id'], 'vid' => $result['vid'], 'ip' => $request->getClientIp()]);
 
         return new JsonResponse(array('status' => 'success'), 201);
     }
@@ -166,6 +166,8 @@ class ApiController extends Controller
         }
 
         $failed = array();
+        $ip = $request->getClientIp();
+        $jobs = [];
         foreach ($contents['downloads'] as $package) {
             $result = $this->getPackageAndVersionId($package['name'], $package['version']);
 
@@ -174,8 +176,9 @@ class ApiController extends Controller
                 continue;
             }
 
-            $this->trackDownload($result['id'], $result['vid'], $request->getClientIp());
+            $jobs[] = ['id' => $result['id'], 'vid' => $result['vid'], 'ip' => $ip];
         }
+        $this->get('packagist.download_manager')->addDownloads($jobs);
 
         if ($failed) {
             return new JsonResponse(array('status' => 'partial', 'message' => 'Packages '.json_encode($failed).' not found'), 200);
@@ -202,21 +205,6 @@ class ApiController extends Controller
         );
     }
 
-    protected function trackDownload($id, $vid, $ip)
-    {
-        $redis = $this->get('snc_redis.default');
-        $manager = $this->get('packagist.download_manager');
-
-        $throttleKey = 'dl:'.$id.':'.$ip.':'.date('Ymd');
-        $requests = $redis->incr($throttleKey);
-        if (1 === $requests) {
-            $redis->expire($throttleKey, 86400);
-        }
-        if ($requests <= 10) {
-            $manager->addDownload($id, $vid);
-        }
-    }
-
     /**
      * Perform the package update
      *

src/Packagist/WebBundle/Model/DownloadManager.php

@@ -107,36 +107,37 @@ class DownloadManager
     }
 
     /**
-     * Tracks a new download by updating the relevant keys.
+     * Tracks downloads by updating the relevant keys.
      *
-     * @param \Packagist\WebBundle\Entity\Package|int $package
-     * @param \Packagist\WebBundle\Entity\Version|int $version
+     * @param array[] an array of arrays containing id (package id), vid (version id) and ip keys
      */
-    public function addDownload($package, $version)
+    public function addDownloads(array $jobs)
     {
-        $redis = $this->redis;
+        $day = date('Ymd');
+        $month = date('Ym');
 
         if (!$this->redisCommandLoaded) {
-            $redis->getProfile()->defineCommand('downloadsIncr', 'Packagist\Redis\DownloadsIncr');
+            $this->redis->getProfile()->defineCommand('downloadsIncr', 'Packagist\Redis\DownloadsIncr');
             $this->redisCommandLoaded = true;
         }
 
-        if ($package instanceof Package) {
-            $package = $package->getId();
-        }
-
-        if ($version instanceof Version) {
-            $version = $version->getId();
+        $args = ['downloads'];
+
+        foreach ($jobs as $job) {
+            $package = $job['id'];
+            $version = $job['vid'];
+
+            // throttle key
+            $args[] = 'dl:'.$package.':'.$job['ip'].':'.$day;
+            // stats keys
+            $args[] = 'dl:'.$package;
+            $args[] = 'dl:'.$package.':'.$month;
+            $args[] = 'dl:'.$package.':'.$day;
+            $args[] = 'dl:'.$package.'-'.$version;
+            $args[] = 'dl:'.$package.'-'.$version.':'.$month;
+            $args[] = 'dl:'.$package.'-'.$version.':'.$day;
         }
 
-        $redis->downloadsIncr(
-            'downloads',
-            'dl:'.$package,
-            'dl:'.$package.':'.date('Ym'),
-            'dl:'.$package.':'.date('Ymd'),
-            'dl:'.$package.'-'.$version,
-            'dl:'.$package.'-'.$version.':'.date('Ym'),
-            'dl:'.$package.'-'.$version.':'.date('Ymd')
-        );
+        $this->redis->downloadsIncr(...$args);
     }
 }

src/Packagist/WebBundle/Tests/Controller/WebControllerTest.php

@@ -196,13 +196,13 @@ class WebControllerTest extends WebTestCase
                 /* @var $downloadManager DownloadManager */
 
                 for ($i = 0; $i < 25; $i += 1) {
-                    $downloadManager->addDownload($twigPackage->getId(), 25);
+                    $downloadManager->addDownloads([['id' => $twigPackage->getId(), 'vid' => 25, 'ip' => '127.0.0.'.random_int(0,255)]]);
                 }
                 for ($i = 0; $i < 12; $i += 1) {
-                    $downloadManager->addDownload($packagistPackage->getId(), 12);
+                    $downloadManager->addDownloads([['id' => $packagistPackage->getId(), 'vid' => 12, 'ip' => '127.0.0.'.random_int(0,255)]]);
                 }
                 for ($i = 0; $i < 25; $i += 1) {
-                    $downloadManager->addDownload($symfonyPackage->getId(), 42);
+                    $downloadManager->addDownloads([['id' => $symfonyPackage->getId(), 'vid' => 42, 'ip' => '127.0.0.'.random_int(0,255)]]);
                 }
 
                 $favoriteManager = $container->get('packagist.favorite_manager');
@@ -346,13 +346,13 @@ class WebControllerTest extends WebTestCase
                 /* @var $downloadManager DownloadManager */
 
                 for ($i = 0; $i < 25; $i += 1) {
-                    $downloadManager->addDownload($twigPackage->getId(), 25);
+                    $downloadManager->addDownloads([['id' => $twigPackage->getId(), 'vid' => 25, 'ip' => '127.0.0.'.random_int(0,255)]]);
                 }
                 for ($i = 0; $i < 12; $i += 1) {
-                    $downloadManager->addDownload($packagistPackage->getId(), 12);
+                    $downloadManager->addDownloads([['id' => $packagistPackage->getId(), 'vid' => 12, 'ip' => '127.0.0.'.random_int(0,255)]]);
                 }
                 for ($i = 0; $i < 42; $i += 1) {
-                    $downloadManager->addDownload($symfonyPackage->getId(), 42);
+                    $downloadManager->addDownloads([['id' => $symfonyPackage->getId(), 'vid' => 42, 'ip' => '127.0.0.'.random_int(0,255)]]);
                 }
             },
             $orderBys