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
Conversation
* @return array | ||
* @throws Exception In case the unserialize fails | ||
*/ | ||
private function unserializeRows($serialized) |
There was a problem hiding this comment.
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
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). |
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 |
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:
We could workaround this by using |
} | ||
} | ||
foreach ($columnsToDelete as $columnToDelete) { |
There was a problem hiding this comment.
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
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 |
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 ingetColumn()
. By changing this to$this->columns[$columnName]
the time dropped to 450 seconds (17%). By usingArrayObject
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 theSerializable
interface and we need to make sure to stay BC with already serialized Row instances in a different format see code.