Faster flatten for some reports #7336
Conversation
tsteur
added
Needs Review
| * @return DataTable|DataTable\Map | ||
| */ | ||
| public function flatten($dataTable) | ||
| public function flatten($dataTable, $recursiveLabelSeparator) |
There was a problem hiding this comment.
$recursiveLabelSeparator is no longer hard coded / hacked in the Flattener, it can be defined by each report now.
tsteur
removed
the
Pull Request WIP
|
Is it a collection of small fixes and improvements or one big refactoring? If it's one big change, it would be interesting to share an overview of what you did because reading the diff is not really easy. Also do you have any numbers (even rough ones) to share regarding the perf improvements? |
|
It is a collection of small fixes and improvements see comments. No big refactoring. I only made many things faster like sorting etc see comments. One big performance improvement was to no longer request each subtable separately but load the expanded datatable at once and then flatten the expanded table. It is hard to show any numbers since in most cases flatten crashed after X seconds/minutes and it was often not even possible to flatten. Not even on demo etc which now loads in < 1 second. Also it depends on the structure of the datatable and of the report. Eg whether there are many first level rows, how many subtables the tables has etc. Basically one can say a flattened table with 25k rows in the UI takes less than 5 seconds, in an API call maybe in even 2 or 3 seconds. On a fast CPU maybe even < 1 or 2 seconds. The more rows a datatable has the better the benefit will be. |
| } | ||
| }); | ||
|
|
||
| // TODO can we remove this one again? |
There was a problem hiding this comment.
Not sure if this TODO should still be there?
There was a problem hiding this comment.
Yes, it should be there. We need to discuss this end of week
|
I'll merge it so we can test on the demo. Lots of small CPU friendly changes 👍 |
Faster flatten for some reports
|
Some UI tests fail because of secondary column sorting: http://builds-artifacts.piwik.org/ui-tests.master/10794.7/screenshot-diffs/diffviewer.html |
|
The UI tests fail because of the other pull request as mentioned there. It is not a bug |
|
the secondary column sort request is followed up in #7401 |
mattab
added
the
c: Performance
refs #5098
I will rebase and squash before merging, please let me know!