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

Jump to bottom

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

Merged
merged 1 commit into from Aug 12, 2015
Merged

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

merged 1 commit into from Aug 12, 2015

Conversation

tsteur
Copy link
Member

@tsteur tsteur 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 c: Performance For when we could improve the performance / speed of Matomo. Needs Review PRs that need a code review labels Jul 28, 2015
@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
Copy link
Member

mattab 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
Copy link
Member Author

tsteur 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
Copy link
Member Author

tsteur commented Jul 30, 2015

I presume you got maybe confused with #8449 ?

@mattab
Copy link
Member

mattab commented Aug 12, 2015

I indeed got confused - LGTM 👍

mattab pushed a commit that referenced this pull request Aug 12, 2015
Merge pull request #8446 from piwik/row_arrayobject2
3e872de 
A Row implementation that is based on ArrayObject which is faster
@mattab mattab merged commit 3e872de into 3.0 Aug 12, 2015
@tsteur tsteur deleted the row_arrayobject2 branch August 19, 2015 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c: Performance For when we could improve the performance / speed of Matomo. Needs Review PRs that need a code review
Projects
None yet
Milestone
3.0.0-b1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tsteur @mattab