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

Further dataTable performance improvements #7502

Merged
merged 2 commits into from Mar 30, 2015
Merged

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Mar 22, 2015

Sorry for having another pull request for this. I noticed those issues while profiling an archiving run that takes 50 minutes and while I was looking into some other things.

This PR will improve performance when using DataTables.

$values[$key] = array($this->getColumnValue($row), $row->getColumn('label'));
}
} else {
foreach ($rows as $key => $row) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only numberSort actually uses a secondary column (label). For all other sort callbacks we do not need the secondary column so we can simply pass the value directly which will make sorting by label faster.

@tsteur tsteur added c: Performance For when we could improve the performance / speed of Matomo. Needs Review PRs that need a code review labels Mar 22, 2015
@tsteur tsteur added this to the Piwik 2.13.0 milestone Mar 23, 2015
// we do not use getRows() as this method might get called 100k times when aggregating many datatables and
// this takes a lot of time.
$row = $tableToSum->getRowFromId(DataTable::ID_SUMMARY_ROW);
if ($row) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly related to this change, maybe it would increase code clarity if DataTable::$summaryRow were stored in DataTable::$rows? I haven't looked to see if this would be do-able or not, maybe something to think about in another pull request.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could point, I think that would simplify many things and even improve performance sometimes. I might have a look at this end of week or next week. Not sure if I will find time though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick note: It might be not as easy. Eg getFirstRow will be more complicated as it should not return the SummaryRow as a first row if there are other rows. Also getLastRow should return the summary row first. Those kinda implementations will get a bit more complicated.

Also sort etc. might get a bit harder since we shouldn't sort the summary row. Here he would have to create a new $rows array without the summary row.

@diosmosis
Copy link
Member

👍

tsteur added a commit that referenced this pull request Mar 30, 2015
Further dataTable performance improvements
@tsteur tsteur merged commit 32639f1 into master Mar 30, 2015
@tsteur tsteur deleted the datatable_tweaks_2 branch March 30, 2015 03:36
@tsteur tsteur self-assigned this Mar 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Performance For when we could improve the performance / speed of Matomo. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants