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 #8446

Merged
merged 1 commit into from Aug 12, 2015

Conversation

Projects
None yet
2 participants
@tsteur
Copy link
Member

commented Jul 28, 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).

Original PR was #7570

@tsteur tsteur added this to the 3.0.0 milestone Jul 28, 2015

@mattab mattab modified the milestones: 3.0.0-b1, 3.0.0 Jul 30, 2015

@mattab

This comment has been minimized.

Copy link
Member

commented Jul 30, 2015

fyi: there is some code missing in this PR, eg. all unit tests were added inyour original PR https://github.com/piwik/piwik/pull/7570/files

@tsteur

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2015

Not quite sure what you mean, tests were already added in another PR that was merged a while ago.

Also see comment in #7570:

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.

We implemented many changes re Serializable etc in another PR so here it is only about switching to ArrayObject and instead of using $this->columns[$name] -> $this[$name]

@tsteur

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2015

I presume you got maybe confused with #8449 ?

@mattab

This comment has been minimized.

Copy link
Member

commented Aug 12, 2015

I indeed got confused - LGTM 👍

mattab pushed a commit that referenced this pull request Aug 12, 2015

Matthieu Aubry
Merge pull request #8446 from piwik/row_arrayobject2
A Row implementation that is based on ArrayObject which is faster

@mattab mattab merged commit 3e872de into 3.0 Aug 12, 2015

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Scrutinizer 25 new issues, 1 updated code elements
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tsteur tsteur deleted the row_arrayobject2 branch Aug 19, 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.