Project

General

Profile

Actions

Task #63394

closed

DatabaseRecordList::createReferenceHtml uses too much memory

Added by Tizian Schmidlin over 9 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
Must have
Category:
Code Cleanup
Target version:
Start date:
2014-11-28
Due date:
% Done:

0%

Estimated time:
TYPO3 Version:
6.2
PHP Version:
5.3
Tags:
Complexity:
Sprint Focus:

Description

Hello there,

I've found an issue concerning the way DatabaseRecordList::createReferenceHtml get's the data and generates the html.

It appears, that in order to create the ref link in the Liste module ALL ref records to a specific set of database entry and table are fetched. This might not be a huge problem in most cases where you have less than 100'000 references on a single element, but as it happens, we do have a fe_group that has 900'000+ referemces to it. I already made sure that the ref index was up to date and it made it even worse, adding another 10'000 records to the refindex table.

So mainly the problem is, that this little function uses more than 512M of RAM to finish:

    /**
     * Creates the HTML for a reference count for the record with the UID $uid
     * in the table $tableName.
     *
     * @param string $tableName
     * @param integer $uid
     * @return string HTML of reference a link, will be empty if there are no
     */
    protected function createReferenceHtml($tableName, $uid) {
        $rows = $GLOBALS['TYPO3_DB']->exec_SELECTgetRows(
            'tablename, recuid, field',
            'sys_refindex',
            'ref_table = ' . $GLOBALS['TYPO3_DB']->fullQuoteStr($tableName, 'sys_refindex') .
            ' AND ref_uid = ' . $uid . ' AND deleted = 0'
        );
        return $this->generateReferenceToolTip($rows, '\'' . $tableName . '\', \'' . $uid . '\'');
    }

So here is the first issue: if fetches all records for the query, that means an array of over 900'000+ entries is created, which itself would probably be enough to kill any PHP script.

But why is that so? I assume it's due to some of Kaspar's legacy code in generateReferenceToolTip (but at this point I'm only assuming).

Now let's look at the generateReferenceToolTip method, found in \TYPO3\CMS\Backend\RecordList\AbstractRecordList:

    /**
     * Generates HTML code for a Reference tooltip out of
     * sys_refindex records you hand over
     *
     * @param array $references array of records from sys_refindex table
     * @param string $launchViewParameter JavaScript String, which will be passed as parameters to top.launchView
     * @return string
     */
    protected function generateReferenceToolTip(array $references, $launchViewParameter = '') {
        $result = array();
        foreach ($references as $reference) {
            $result[] = $reference['tablename'] . ':' . $reference['recuid'] . ':' . $reference['field'];
            if (strlen(implode(' / ', $result)) >= 100) {
                break;
            }
        }
        if (empty($result)) {
            $htmlCode = '-';
        } else {
            $htmlCode = '<a href="#"';
            if ($launchViewParameter !== '') {
                $htmlCode .= ' onclick="' . htmlspecialchars(('top.launchView(' . $launchViewParameter . '); return false;')) . '"';
            }
            $htmlCode .= ' title="' . htmlspecialchars(GeneralUtility::fixed_lgd_cs(implode(' / ', $result), 100)) . '">';
            $htmlCode .= count($references);
            $htmlCode .= '</a>';
        }
        return $htmlCode;
    }

So basically, we concatenate the first 5-20 records (in average) only to check, if we have to write a dash or if we can proceed to the html code and to make a title. This looks a bit over the top to me. Checking the emptiness could be solved easier by simply counting the $references which is done anyways further down. Now this might also be an issue, since counting the length of such a big array may cause PHP also to crash. The link title could be simply reduced to a static translated text à la "show references (X)" and put the count in brackets.

So my fix here would be as follows:

  1. modify the query in createReferenceHtml to directly count the results from the database
  2. modify the behaviour of generateReferenceToolTip to receive the $references parameter which for compatibility reasons should still be an array that only contains one entry "referenceCount" that is used through the whole logic.
This will:
  • save memory
  • save time
  • make the code more readable
  • get rid of some really bad code

I provided my own implementation as an example.

Best Regards
Tizian


Files

DatabaseRecordList.php (1.64 KB) DatabaseRecordList.php Implementation of new DatabaseRecordList logic Tizian Schmidlin, 2014-11-28 11:05
Actions #1

Updated by Mathias Schreiber over 9 years ago

  • Target version changed from next-patchlevel to 7.1 (Cleanup)
Actions #2

Updated by Gerrit Code Review over 9 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/35744

Actions #3

Updated by Mathias Schreiber over 9 years ago

  • Tracker changed from Bug to Task
  • Assignee set to Mathias Schreiber

Since this introduced a change in behavior we change this to a task.
Still: good catch :)

Actions #4

Updated by Gerrit Code Review over 9 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/35744

Actions #5

Updated by Wouter Wolters over 9 years ago

  • Status changed from Under Review to Resolved
Actions #6

Updated by Riccardo De Contardi over 6 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF