From 1924f3c1d711a0fc54abbae160c7067d9942cae4 Mon Sep 17 00:00:00 2001 From: MarkBaker Date: Thu, 28 Apr 2022 12:43:05 +0200 Subject: [PATCH] Improved memory performance for getSortedCoordinates() in the Cell Collection; with minimal execution time overhead. This is an important method, as it's called by the Reference Helper and for the Writer save() method; so users should see that improvement. --- src/PhpSpreadsheet/Collection/Cells.php | 65 ++++++++++--------- .../Collection/CellsTest.php | 28 +++++--- 2 files changed, 54 insertions(+), 39 deletions(-) diff --git a/src/PhpSpreadsheet/Collection/Cells.php b/src/PhpSpreadsheet/Collection/Cells.php index d49f5457..cc3d17e8 100644 --- a/src/PhpSpreadsheet/Collection/Cells.php +++ b/src/PhpSpreadsheet/Collection/Cells.php @@ -4,6 +4,7 @@ namespace PhpOffice\PhpSpreadsheet\Collection; use Generator; use PhpOffice\PhpSpreadsheet\Cell\Cell; +use PhpOffice\PhpSpreadsheet\Cell\Coordinate; use PhpOffice\PhpSpreadsheet\Exception as PhpSpreadsheetException; use PhpOffice\PhpSpreadsheet\Settings; use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet; @@ -150,41 +151,16 @@ class Cells public function getSortedCoordinates() { $sortKeys = []; - foreach ($this->getCoordinates() as $coord) { - sscanf($coord, '%[A-Z]%d', $column, $row); - $sortKeys[sprintf('%09d%3s', $row, $column)] = $coord; + foreach ($this->getCoordinates() as $coordinate) { + sscanf($coordinate, '%[A-Z]%d', $column, $row); + $key = (--$row * 16384) + Coordinate::columnIndexFromString($column); + $sortKeys[$key] = $coordinate; } ksort($sortKeys); return array_values($sortKeys); } - /** - * Get highest worksheet column and highest row that have cell records. - * - * @return array Highest column name and highest row number - */ - public function getHighestRowAndColumn() - { - // Lookup highest column and highest row - $col = ['A' => '1A']; - $row = [1]; - foreach ($this->getCoordinates() as $coord) { - sscanf($coord, '%[A-Z]%d', $c, $r); - $row[$r] = $r; - $col[$c] = strlen($c) . $c; - } - - // Determine highest column and row - $highestRow = max($row); - $highestColumn = substr((string) @max($col), 1); - - return [ - 'row' => $highestRow, - 'column' => $highestColumn, - ]; - } - /** * Return the cell coordinate of the currently active cell object. * @@ -219,6 +195,32 @@ class Cells return (int) $row; } + /** + * Get highest worksheet column and highest row that have cell records. + * + * @return array Highest column name and highest row number + */ + public function getHighestRowAndColumn() + { + // Lookup highest column and highest row + $columns = ['1A']; + $rows = [1]; + foreach ($this->getCoordinates() as $coordinate) { + sscanf($coordinate, '%[A-Z]%d', $column, $row); + $rows[$row] = $rows[$row] ?? $row; + $columns[$column] = $columns[$column] ?? strlen($column) . $column; + } + + // Determine highest column and row + $highestRow = max($rows); + $highestColumn = substr((string) max($columns), 1); + + return [ + 'row' => $highestRow, + 'column' => $highestColumn, + ]; + } + /** * Get highest worksheet column. * @@ -239,7 +241,8 @@ class Cells if ($r != $row) { continue; } - $maxColumn = max($maxColumn, strlen($c) . $c); + $sortableColum = strlen($c) . $c; + $maxColumn = $maxColumn > $sortableColum ? $maxColumn : $sortableColum; } return substr($maxColumn, 1); @@ -265,7 +268,7 @@ class Cells if ($c != $column) { continue; } - $maxRow = max($maxRow, $r); + $maxRow = ($maxRow > $r) ? $maxRow : $r; } return $maxRow; diff --git a/tests/PhpSpreadsheetTests/Collection/CellsTest.php b/tests/PhpSpreadsheetTests/Collection/CellsTest.php index 0183e16a..866f4117 100644 --- a/tests/PhpSpreadsheetTests/Collection/CellsTest.php +++ b/tests/PhpSpreadsheetTests/Collection/CellsTest.php @@ -29,16 +29,28 @@ class CellsTest extends TestCase self::assertSame($cell1, $collection->add('B2', $cell1), 'adding a cell should return the cell'); // Assert cell presence - self::assertEquals(['B2'], $collection->getCoordinates(), 'cell list should contains the cell'); - self::assertEquals(['B2'], $collection->getSortedCoordinates(), 'sorted cell list contains the cell'); + self::assertEquals(['B2'], $collection->getCoordinates(), 'cell list should contains the B2 cell'); + self::assertEquals(['B2'], $collection->getSortedCoordinates(), 'sorted cell list contains the B2 cell'); self::assertSame($cell1, $collection->get('B2'), 'should get exact same object'); - self::assertTrue($collection->has('B2'), 'cell should exists'); + self::assertTrue($collection->has('B2'), 'B2 cell should exists'); // Add a second cell $cell2 = $sheet->getCell('A1'); self::assertSame($cell2, $collection->add('A1', $cell2), 'adding a second cell should return the cell'); - self::assertEquals(['B2', 'A1'], $collection->getCoordinates(), 'cell list should contains the cell'); - self::assertEquals(['A1', 'B2'], $collection->getSortedCoordinates(), 'sorted cell list contains the cell'); + self::assertEquals(['B2', 'A1'], $collection->getCoordinates(), 'cell list should contains the second cell'); + self::assertEquals(['A1', 'B2'], $collection->getSortedCoordinates(), 'sorted cell list contains the second cell'); + + // Add a third cell + $cell3 = $sheet->getCell('AA1'); + self::assertSame($cell3, $collection->add('AA1', $cell3), 'adding a third cell should return the cell'); + self::assertEquals(['B2', 'A1', 'AA1'], $collection->getCoordinates(), 'cell list should contains the third cell'); + self::assertEquals(['A1', 'AA1', 'B2'], $collection->getSortedCoordinates(), 'sorted cell list contains the third cell'); + + // Add a fourth cell + $cell4 = $sheet->getCell('Z1'); + self::assertSame($cell4, $collection->add('Z1', $cell4), 'adding a fourth cell should return the cell'); + self::assertEquals(['B2', 'A1', 'AA1', 'Z1'], $collection->getCoordinates(), 'cell list should contains the fourth cell'); + self::assertEquals(['A1', 'Z1', 'AA1', 'B2'], $collection->getSortedCoordinates(), 'sorted cell list contains the fourth cell'); // Assert collection copy $sheet2 = $spreadsheet->createSheet(); @@ -53,7 +65,7 @@ class CellsTest extends TestCase // Assert deletion $collection->delete('B2'); self::assertFalse($collection->has('B2'), 'cell should have been deleted'); - self::assertEquals(['A1'], $collection->getCoordinates(), 'cell list should contains the cell'); + self::assertEquals(['A1', 'AA1', 'Z1'], $collection->getCoordinates(), 'cell list should still contains the A1,Z1 and A11 cells'); // Assert update $cell2 = $sheet->getCell('A1'); @@ -61,8 +73,8 @@ class CellsTest extends TestCase self::assertSame($cell2, $collection->update($cell2), 'should update existing cell'); $cell3 = $sheet->getCell('C3'); - self::assertSame($cell3, $collection->update($cell3), 'should silently add non-existing cell'); - self::assertEquals(['A1', 'C3'], $collection->getCoordinates(), 'cell list should contains the cell'); + self::assertSame($cell3, $collection->update($cell3), 'should silently add non-existing C3 cell'); + self::assertEquals(['A1', 'AA1', 'Z1', 'C3'], $collection->getCoordinates(), 'cell list should contains the C3 cell'); } public function testCacheLastCell(): void