Suboptimal memory usage in PhpSpreadsheet\Reader\Xlsx->readColumnsAndRowsAttributes()

[tomi-heiskanen]

This is: a bug report & suggested patch

What is the expected behavior?

Not to consume double amount memory which would be needed for processing.

What is the current behavior?

The memory usage is unnecessarily doubled as row&column attributes are first read into arrays and then set into the worksheet attributes.

What are the steps to reproduce?

1. Attempt to read a very large Excel file (hundreds of columns and 1M+ rows)
2. load() runs out of memory (unless gigabytes of memory is allowed for processing)

Which versions of PhpSpreadsheet and PHP are affected?

At least from 1.4.0 until current develop-branch.

Suggestion how to fix it

I could not create a pull request due to Github a permission issue, but please see patched version of the function which does not read everything into arrays first, but rather sets the attributes while processing. Patched Xlsx.php: https://gist.github.com/tomi-heiskanen/a1d0e3d376d1b019f6072eda33bd0c11

[PowerKiKi]

That would be a very welcome improvement. Could you please give it another try at creating a PR ? As long as you have a GitHub account, you should be able to create PR for any projects. See https://help.github.com/articles/creating-a-pull-request-from-a-fork/

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

[PowerKiKi]

I tried to apply your suggestion on current develop branch. I ended up with the following patch, but I could not measure any memory gain. So unless someone can proof that it is useful, it won't be merged.

From 962b5fcde082973886f99adb844715245a8d2e78 Mon Sep 17 00:00:00 2001
Date: Mon, 10 Dec 2018 23:22:29 +1100
Subject: [PATCH] wip from
 https://github.com/PHPOffice/PhpSpreadsheet/issues/648

---
 src/PhpSpreadsheet/Reader/Xlsx.php | 78 +++++++++++++-----------------
 t.php                              | 11 +++++
 2 files changed, 44 insertions(+), 45 deletions(-)
 create mode 100644 t.php

diff --git src/PhpSpreadsheet/Reader/Xlsx.php src/PhpSpreadsheet/Reader/Xlsx.php
index 335f5d7e..6abe4adb 100644
--- src/PhpSpreadsheet/Reader/Xlsx.php
+++ src/PhpSpreadsheet/Reader/Xlsx.php
@@ -2520,24 +2520,38 @@ class Xlsx extends BaseReader
      */
     private function readColumnsAndRowsAttributes(SimpleXMLElement $xmlSheet, Worksheet $docSheet)
     {
-        $columnsAttributes = [];
-        $rowsAttributes = [];
+        $readFilter = (\get_class($this->getReadFilter()) !== DefaultReadFilter::class ? $this->getReadFilter() : null);
+
+        $columns = [];
         if (isset($xmlSheet->cols) && !$this->readDataOnly) {
             foreach ($xmlSheet->cols->col as $col) {
                 for ($i = (int) ($col['min']); $i <= (int) ($col['max']); ++$i) {
+                    $coordColumn = Coordinate::stringFromColumnIndex($i);
+                    $columnAttributes = [];
                     if ($col['style'] && !$this->readDataOnly) {
-                        $columnsAttributes[Coordinate::stringFromColumnIndex($i)]['xfIndex'] = (int) $col['style'];
+                        $columnAttributes['xfIndex'] = (int) $col['style'];
                     }
                     if (self::boolean($col['hidden'])) {
-                        $columnsAttributes[Coordinate::stringFromColumnIndex($i)]['visible'] = false;
+                        $columnAttributes['visible'] = false;
                     }
                     if (self::boolean($col['collapsed'])) {
-                        $columnsAttributes[Coordinate::stringFromColumnIndex($i)]['collapsed'] = true;
+                        $columnAttributes['collapsed'] = true;
                     }
                     if ($col['outlineLevel'] > 0) {
-                        $columnsAttributes[Coordinate::stringFromColumnIndex($i)]['outlineLevel'] = (int) $col['outlineLevel'];
+                        $columnAttributes['outlineLevel'] = (int) $col['outlineLevel'];
+                    }
+                    $columnAttributes['width'] = (float) $col['width'];
+                    array_push($columns, $coordColumn);
+
+                    if ($readFilter !== null) {
+                        foreach ($xmlSheet->sheetData->row as $row) {
+                            if (!$this->getReadFilter()->readCell($coordColumn, (int) $row['r'], $docSheet->getTitle())) {
+                                continue 2;
+                            }
+                        }
                     }
-                    $columnsAttributes[Coordinate::stringFromColumnIndex($i)]['width'] = (float) $col['width'];
+
+                    $this->setColumnAttributes($docSheet, $coordColumn, $columnAttributes);
 
                     if ((int) ($col['max']) == 16384) {
                         break;
@@ -2548,56 +2562,30 @@ class Xlsx extends BaseReader
 
         if ($xmlSheet && $xmlSheet->sheetData && $xmlSheet->sheetData->row) {
             foreach ($xmlSheet->sheetData->row as $row) {
+                $rowAttributes = [];
                 if ($row['ht'] && !$this->readDataOnly) {
-                    $rowsAttributes[(int) $row['r']]['rowHeight'] = (float) $row['ht'];
+                    $rowAttributes['rowHeight'] = (float) $row['ht'];
                 }
                 if (self::boolean($row['hidden']) && !$this->readDataOnly) {
-                    $rowsAttributes[(int) $row['r']]['visible'] = false;
+                    $rowAttributes['visible'] = false;
                 }
                 if (self::boolean($row['collapsed'])) {
-                    $rowsAttributes[(int) $row['r']]['collapsed'] = true;
+                    $rowAttributes['collapsed'] = true;
                 }
                 if ($row['outlineLevel'] > 0) {
-                    $rowsAttributes[(int) $row['r']]['outlineLevel'] = (int) $row['outlineLevel'];
+                    $rowAttributes['outlineLevel'] = (int) $row['outlineLevel'];
                 }
                 if ($row['s'] && !$this->readDataOnly) {
-                    $rowsAttributes[(int) $row['r']]['xfIndex'] = (int) $row['s'];
-                }
-            }
-        }
-
-        $readFilter = (\get_class($this->getReadFilter()) !== DefaultReadFilter::class ? $this->getReadFilter() : null);
-
-        // set columns/rows attributes
-        $columnsAttributesSet = [];
-        $rowsAttributesSet = [];
-        foreach ($columnsAttributes as $coordColumn => $columnAttributes) {
-            if ($readFilter !== null) {
-                foreach ($rowsAttributes as $coordRow => $rowAttributes) {
-                    if (!$readFilter->readCell($coordColumn, $coordRow, $docSheet->getTitle())) {
-                        continue 2;
-                    }
+                    $rowAttributes['xfIndex'] = (int) $row['s'];
                 }
-            }
-
-            if (!isset($columnsAttributesSet[$coordColumn])) {
-                $this->setColumnAttributes($docSheet, $coordColumn, $columnAttributes);
-                $columnsAttributesSet[$coordColumn] = true;
-            }
-        }
-
-        foreach ($rowsAttributes as $coordRow => $rowAttributes) {
-            if ($readFilter !== null) {
-                foreach ($columnsAttributes as $coordColumn => $columnAttributes) {
-                    if (!$readFilter->readCell($coordColumn, $coordRow, $docSheet->getTitle())) {
-                        continue 2;
+                if ($readFilter !== null) {
+                    foreach ($columns as $coordColumn) {
+                        if (!$this->getReadFilter()->readCell($coordColumn, (int) $row['r'], $docSheet->getTitle())) {
+                            continue 2;
+                        }
                     }
                 }
-            }
-
-            if (!isset($rowsAttributesSet[$coordRow])) {
-                $this->setRowAttributes($docSheet, $coordRow, $rowAttributes);
-                $rowsAttributesSet[$coordRow] = true;
+                $this->setRowAttributes($docSheet, (int) $row['r'], $rowAttributes);
             }
         }
     }
diff --git t.php t.php
new file mode 100644
index 00000000..6c0afb62
--- /dev/null
+++ t.php
@@ -0,0 +1,11 @@
+<?php
+
+use PhpOffice\PhpSpreadsheet\IOFactory;
+
+require 'vendor/autoload.php';
+
+$filename = '/tmp/phpspreadsheet/06_Largescale.xlsx';
+
+$s2 = IOFactory::load($filename);
+var_dump(memory_get_peak_usage() / 1024 / 1024);
+var_dump(memory_get_peak_usage(true) / 1024 / 1024);
-- 
2.17.1