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

A Row implementation that is based on ArrayObject which is faster #7570

Closed
wants to merge 2 commits into from

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Mar 29, 2015

Currently, a Row's columns are accessed like this: $this->c[self::COLUMNS][$columnName]. When archiving a big year instance we spend about 900 seconds (33%) just in getColumn(). By changing this to $this->columns[$columnName] the time dropped to 450 seconds (17%). By using ArrayObject the time drops by another 250 seconds. A difference in speed is not really noticeable in normal reports but when archiving aggregated reports (week, month, year, ranges).

Unfortunately, there is a huge disadvantage when using ArrayObject. It is not debuggable in PHPStorm and other IDE's as it is not supported by Xdebug see http://bugs.xdebug.org/view.php?id=996 and also kinda related http://bugs.xdebug.org/view.php?id=686 . Those issues are open since 2013 and 2011 so we can't expect a fix for this soon.

I'm issuing this pull request so we still have the code in case those bugs are resolved.

Please note in this PR there are also some changes to subtableId etc. which will be probably in master soon anyway. I didn't make the effort to split those changes as we only wanted to still have the code in general in case those bugs will be fixed in Xdebug. It wasn't easy to implement it based on ArrayObject since it implements the Serializable interface and we need to make sure to stay BC with already serialized Row instances in a different format see code.

* @return array
* @throws Exception In case the unserialize fails
*/
private function unserializeRows($serialized)
Copy link
Member Author

Choose a reason for hiding this comment

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

ArrayObject implements the Serializable interface therefore we cannot simple unserialize the serialized Row instances see comment. This was the tricky part in making it work with ArrayObject

@tsteur tsteur added the c: Performance For when we could improve the performance / speed of Matomo. label Mar 29, 2015
@mattab mattab added this to the Mid term milestone Mar 30, 2015
@mattab
Copy link
Member

mattab commented Mar 30, 2015

It is not debuggable in PHPStorm and other IDE's as it is not supported by Xdebug see http://bugs.xdebug.org/view.php?id=996

I've commented on this issue and also emailed Derick privately to offer to sponsor the work on this xdebug feature. I don't think there are other next steps for this task for now.

this Pull request may be opened for a long time (ie. until Xdebug implements the feature).

@mattab
Copy link
Member

mattab commented Apr 6, 2015

Good news: following our request and sponsor offer, Derick from Xdebug has implemented the ArrayObject inspection feature and the ticket was closed. Xdebug 2.3.3 is not yet released but should be in next weeks, then we can merge this PR.

Moving to 2.14.0

@mattab mattab modified the milestones: Piwik 2.14.0, Mid term Apr 6, 2015
@tsteur
Copy link
Member Author

tsteur commented Apr 28, 2015

Actually, I will move this to Piwik 3.0 which we will work on "soon" anyway. Xdebug 2.3.3 is not released yet anyway and I also remember one issue with PHP 5.3 where the following is happening:

$row->setColumn('test', null);
$column = $row->getColumn('test');
// got: $column === null expected: $column === false

We could workaround this by using $this->offsetExists($columnName) instead of isset($this[$columnName]) but it would make it a bit slower again...

@tsteur tsteur modified the milestones: Piwik 3.0.0, Piwik 2.14.0 Apr 28, 2015
}
}
foreach ($columnsToDelete as $columnToDelete) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a empty line above this one

@tsteur
Copy link
Member Author

tsteur commented Jul 28, 2015

A new xdebug feature was released and we have a 3.0 branch. I will issue a new PR for this and we can merge it soon

@tsteur tsteur closed this Jul 28, 2015
@tsteur tsteur deleted the row_arrayobject branch July 29, 2015 12:22
@mattab mattab modified the milestones: 3.0.0, 3.0.0-b1 Jul 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants