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

Projects
None yet
3 participants
@tsteur
Copy link
Member

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)

This comment has been minimized.

Copy link
@tsteur

tsteur Mar 29, 2015

Author Member

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

foreach ($rows as $id => $row) {
if (isset($row->c)) {
$row = $row->c; // before Piwik 2.13
}

This comment has been minimized.

Copy link
@tsteur

tsteur Mar 29, 2015

Author Member

Pre Piwik 2.13 we have Row::$c, afterwards we just have an array of rows

if ($this->isSubtableLoaded()) {
Manager::getInstance()->deleteTable($this->getIdSubDataTable());
$this->c[self::DATATABLE_ASSOCIATED] = null;
if ($this->isSubtableLoaded) {

This comment has been minimized.

Copy link
@tsteur

tsteur Mar 29, 2015

Author Member

As mentioned in the PR the subtable changes are not really relevant to this PR

@mattab mattab added this to the Mid term milestone Mar 30, 2015

@mattab

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

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) {

This comment has been minimized.

Copy link
@fabiocarneiro

fabiocarneiro May 20, 2015

Contributor

add a empty line above this one

if ($this->subtableIdWasNegativeBeforeSerialize) {
$this->switchFlagSubtableIsLoaded();
$this->subtableIdWasNegativeBeforeSerialize = false;
if (isset($row[self::DATATABLE_ASSOCIATED])) {

This comment has been minimized.

Copy link
@fabiocarneiro

fabiocarneiro May 20, 2015

Contributor

Use early return instead.

if (! isset($row[self::DATATABLE_ASSOCIATED])) {
    return;
}

if ($row[self::DATATABLE_ASSOCIATED] instanceof DataTable) {
    $this->setSubtable($row[self::DATATABLE_ASSOCIATED]);
    return;
}

$this->subtableId = $row[self::DATATABLE_ASSOCIATED];
@tsteur

This comment has been minimized.

Copy link
Member Author

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 Jul 29, 2015

@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
You can’t perform that action at this time.