Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Less PHP Memory usage! Truncate filter should not be applied recursively when sub-datatables are not loaded #3207

Closed
mattab opened this issue Jun 8, 2012 · 14 comments
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Jun 8, 2012

In Truncate.php#L42, Truncate is applied recursively.

When expanded = 0, sub-datatables are not available in Piwik_DataTable_Manager->tables.

In that case, Truncate filter should not try to load sub-datatables as in Truncate.php#L43 :

Piwik_DataTable_Manager::getInstance()->getTable($idSubTable);

It works 'fine' when *$this->c[corresponds to an empty entry in https://github.com/piwik/piwik/blob/master/core/DataTable/Manager.php#L43 Piwik_DataTable_Manager->tables because an exception is raised in Manager.php#L75 and silenced in Truncate.php#L45.

However, in some random but legitimate cases, $this->c[does correspond to a non-empty entry in https://github.com/piwik/piwik/blob/master/core/DataTable/Manager.php#L43 Piwik_DataTable_Manager->tables. However, this non-empty entry has nothing to do with the sub-datable as they are not loaded in memory.

The worst case scenario, as happening for users generating reports, see forum post, is when $this->c[self::DATATABLE_ASSOCIATED] of a Piwik_DataTable_Row is equal to its enclosing Piwik_DataTable->currendId. This is when the infinite recursion happens.

All of this is fine, when expanded = 1 because sub-datatable are loaded in memory and their IDs *ie. $this->c[are synchronized with https://github.com/piwik/piwik/blob/master/core/DataTable/Manager.php#L43 Piwik_DataTable_Manager->tables, see Piwik_Archive_Single L355 :

// and update the subtableID so that it matches the newly instantiated table 

How to fix ?

Truncate filter should not perform recursive filtering when sub-datatables are not requested (ie. expended = 0)

Truncate filter should use Piwik_DataTable_Filter->filterSubTable which checks if filters should be applied recursively :

71          if(!$this->enableRecursive)
72          {
73              return;
74          }
@mattab
Copy link
Member Author

mattab commented Jun 21, 2012

Reported by many users in http://forum.piwik.org/read.php?2,90170 as well

@julienmoumne
Copy link
Member

As described in comment:1:ticket:3191, one issue we found is infinite nesting calls, see http://pastebin.com/Kdrkb7V9

Applying the following patch http://forum.piwik.org/read.php?2,90170,page=2#msg-90681 solves the infinite nesting.

However, we have yet to know :

  • how a reflective cycle can happen on a DataTable
  • how we should fix this
  • if this issue is the same for all users reporting issues with scheduled reports, e.g: Automatic reports are broken #3254

@mattab
Copy link
Member Author

mattab commented Jun 27, 2012

Thank you Julien for the research and follow up on the forum!!!
Great stuff :)

There is indeed a deeper bug that we need to somehow find out: how can it happen that we end up with a Subtable with the same ID as the parent table??!
This should not be possible. Maybe we need to prevent this at the datatable level somehow.

If we don't fix the root issue, we will have to patch as you did all other filters & codes that use subtables the same way...

It would be very good if we could understand how such buggy data happens & replicate the bug... I asked in the thread

@mattab
Copy link
Member Author

mattab commented Jul 27, 2012

Thank you Julien for this great bug hunt and kuddos on finding the prize... I agree with your clear description & the suggestion for a fix.

Did you quickly check that all filters are "safe" with this regards?

@julienmoumne
Copy link
Member

Did you quickly check that all filters are "safe" with this regards?

Running a search on 'getIdSubDataTable' in directory 'core\DataTable\Filter' yields two results :

  • Truncate.php
  • PatternRecursive.php

The latter is a recursive version of Pattern.php.

I am wondering if PatternRecursive.php was necessary. Pattern.php could maybe be rewritten to support recursion using Piwik_DataTable_Filter->filterSubTable.

Anyway, PatternRecursive.php should only be used when dealing with expanded results. Meaning the issue we have with Truncate.php should not occur with this filter.

@mattab
Copy link
Member Author

mattab commented Jul 30, 2012

Thanks for confirming! Yes PatternRecursive should be Pattern and is not necessary indeed.

@mattab
Copy link
Member Author

mattab commented Jul 30, 2012

(In [6593]) Refs #3207 - Fixing (?) a 4 year old issue with the DataTable_Manager:

  • Now a DataTable_Row knows if the sub-datatable was loaded in memory, which is useful when __destruct() is called to free memory

I think the ticket is closed but pending my code being reviewed
Also, we could still get rid of PatternRecursive

@mattab
Copy link
Member Author

mattab commented Jul 30, 2012

(In [6594]) Refs #3207 - Fixing regressions

@mattab
Copy link
Member Author

mattab commented Jul 30, 2012

The build is failing but all_tests pass on my box! I'll look into it tomorrow unless you find the bug :-)

@mattab
Copy link
Member Author

mattab commented Jul 31, 2012

(In [6598]) Refs #3207 ensure that __destruct() is called on the subtables

@mattab
Copy link
Member Author

mattab commented Jul 31, 2012

(In [6606]) Refs #3084 This should fix it! but I'll wait for @owen confirmation,
Refs #3207 Now calling destroy() prior to setTableDeleted as expected

@mattab
Copy link
Member Author

mattab commented Jul 31, 2012

(In [6607]) Refs #3207 the "hack" in DataTable_Row to set tables loaded in memory with a negative ID implies that the table IDs are != 0 --- thanks Julien for the review and good catch

@mattab
Copy link
Member Author

mattab commented Jul 31, 2012

It looks like it's now fixed. Please, reopen or comment if you find any problem or suggestion!

@julienmoumne
Copy link
Member

(In [6654]) refs #3207 unit tests

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Development

No branches or pull requests

2 participants