From 8af1c62bd27488c09c60026ce9dba2e92668da1f Mon Sep 17 00:00:00 2001
From: Joseph Bielawski <stloyd@users.noreply.github.com>
Date: Mon, 30 Oct 2023 17:51:13 +0100
Subject: [PATCH] Improve performance for a few `Rows` methods (#671)

* Improve performance for a few `Rows` methods
Covered: `find`, `findOne`, `first`, `partitionBy`, `take`, `takeRight`

* Rework benchmark GH action to use artifact for baseline
---
 .github/workflows/test-benchmark.yml          | 14 ++---
 src/core/etl/src/Flow/ETL/Rows.php            | 54 ++++---------------
 .../tests/Flow/ETL/Tests/Unit/RowsTest.php    |  2 +-
 3 files changed, 16 insertions(+), 54 deletions(-)

diff --git a/.github/workflows/test-benchmark.yml b/.github/workflows/test-benchmark.yml
index 0dbce8259..e5946c58c 100644
--- a/.github/workflows/test-benchmark.yml
+++ b/.github/workflows/test-benchmark.yml
@@ -60,16 +60,12 @@ jobs:
       - name: "Install locked dependencies"
         run: "composer install --no-interaction --no-progress"
 
-      - name: "Restore PHPBench cache"
-        uses: "actions/cache@v3"
+      - name: "Download phpbench benchmarks artifact"
+        uses: dawidd6/action-download-artifact@v2
         with:
-          path: "var/phpbench"
-          key: "php-${{ matrix.php-version }}-1.x-phpbench-"
-          restore-keys: |
-            php-${{ matrix.php-version }}-1.x-phpbench-
-
-      - name: "Clean previous summary"
-        run: rm -rf ./var/phpbench/summary.txt
+          workflow: benchmark-baseline.yml
+          name: phpbench-baseline
+          path: ./var/phpbench/
 
       - name: "Execute benchmarks"
         id: init_comment
diff --git a/src/core/etl/src/Flow/ETL/Rows.php b/src/core/etl/src/Flow/ETL/Rows.php
index aad3c797d..4adb7abb2 100644
--- a/src/core/etl/src/Flow/ETL/Rows.php
+++ b/src/core/etl/src/Flow/ETL/Rows.php
@@ -190,9 +190,7 @@ public function filter(callable $callable) : self
 
     public function find(callable $callable) : self
     {
-        $rows = $this->rows;
-
-        if (!\count($rows)) {
+        if (0 === $this->count()) {
             return new self();
         }
 
@@ -209,34 +207,18 @@ public function find(callable $callable) : self
 
     public function findOne(callable $callable) : ?Row
     {
-        $rows = $this->rows;
-
-        if (!\count($rows)) {
-            return null;
-        }
-
-        $rows = [];
-
         foreach ($this->rows as $row) {
             if ($callable($row)) {
-                $rows[] = $row;
+                return $row;
             }
         }
 
-        if ([] !== $rows) {
-            return \current($rows);
-        }
-
         return null;
     }
 
     public function first() : Row
     {
-        if (empty($this->rows)) {
-            throw new RuntimeException('First row does not exist in empty collection');
-        }
-
-        return $this->rows[0];
+        return $this->rows[0] ?? throw new RuntimeException('First row does not exist in empty collection');
     }
 
     /**
@@ -509,11 +491,11 @@ public function offsetSet(mixed $offset, mixed $value) : void
     /**
      * @param int $offset
      *
-     * @throws InvalidArgumentException
+     * @throws RuntimeException
      */
     public function offsetUnset(mixed $offset) : void
     {
-        throw new RuntimeException('In order to add new rows use Rows::remove(int $offset) : self');
+        throw new RuntimeException('In order to remove rows use Rows::remove(int $offset) : self');
     }
 
     /**
@@ -526,9 +508,7 @@ public function offsetUnset(mixed $offset) : void
      */
     public function partitionBy(string|Reference $entry, string|Reference ...$entries) : array
     {
-        \array_unshift($entries, $entry);
-
-        $refs = References::init(...$entries);
+        $refs = References::init($entry, ...$entries);
 
         /** @var array<string, array<mixed>> $partitions */
         $partitions = [];
@@ -537,10 +517,10 @@ public function partitionBy(string|Reference $entry, string|Reference ...$entrie
             $partitionEntryValues = [];
 
             foreach ($this->rows as $row) {
-                $partitionEntryValues[] = $row->get($ref)->value();
+                $partitionEntryValues[$row->get($ref)->value()] = true;
             }
 
-            $partitions[$ref->name()] = \array_unique($partitionEntryValues);
+            $partitions[$ref->name()] = \array_keys($partitionEntryValues);
         }
 
         /**
@@ -723,26 +703,12 @@ public function sortEntries() : self
 
     public function take(int $size) : self
     {
-        $rows = $this->rows;
-        $newRows = [];
-
-        for ($i = 0; $i < $size; $i++) {
-            $newRows[] = \array_shift($rows);
-        }
-
-        return new self(...\array_filter($newRows));
+        return new self(...\array_slice($this->rows, 0, $size));
     }
 
     public function takeRight(int $size) : self
     {
-        $rows = $this->rows;
-        $newRows = [];
-
-        for ($i = 0; $i < $size; $i++) {
-            $newRows[] = \array_pop($rows);
-        }
-
-        return new self(...\array_filter($newRows));
+        return new self(...\array_reverse(\array_slice($this->rows, -$size, $size)));
     }
 
     /**
diff --git a/src/core/etl/tests/Flow/ETL/Tests/Unit/RowsTest.php b/src/core/etl/tests/Flow/ETL/Tests/Unit/RowsTest.php
index 24b1afbfd..a7903a581 100644
--- a/src/core/etl/tests/Flow/ETL/Tests/Unit/RowsTest.php
+++ b/src/core/etl/tests/Flow/ETL/Tests/Unit/RowsTest.php
@@ -158,7 +158,7 @@ public function test_array_access_set() : void
     public function test_array_access_unset() : void
     {
         $this->expectException(RuntimeException::class);
-        $this->expectExceptionMessage('In order to add new rows use Rows::remove(int $offset) : self');
+        $this->expectExceptionMessage('In order to remove rows use Rows::remove(int $offset) : self');
         $rows = new Rows(Row::create(new IntegerEntry('id', 1)));
         unset($rows[0]);
     }