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

Improve perforamce #294

Merged
merged 9 commits into from Apr 28, 2016

Conversation

Projects
None yet
2 participants
@mfairchild365
Copy link
Contributor

mfairchild365 commented Apr 27, 2016

This actually does two things to improve performance

  1. Drastically improve query performance for media privacy checks within the media list. Reduces from ~5 seconds to ~.01s. In order to accomplish this, I had implement native sql, as dql (doctrine) doesn't allow for complicated queries like this.
  2. Drastically improve general performance by preventing the UNL_MediaHub_Controller::run() method from being executed twice for every request. this would essentially take the entire request and run it twice.

In total, this PR can bring page load in certain circumstances down from >10 seconds to about 1 second

mfairchild365 added some commits Apr 27, 2016

Use an existing abstraction pattern
Rather than using separate NativeSQL and DQL lists, just use one and let them decide what kind of query to run.
Prevent the controller from running twice
This was duplicating queries, making things twice as slow. Savvy Turbo would render the 'controller' with the custom template, but first run the `run()` method on it (again). yeesh.
@@ -52,5 +52,8 @@
"post-install-cmd": [
"if [ -d ffmpeg ]; then echo 'ffmpeg is already installed'; else mkdir ffmpeg && wget -q -O ffmpeg-download.tar.xz http://johnvansickle.com/ffmpeg/releases/ffmpeg-release-64bit-static.tar.xz && tar xf ffmpeg-download.tar.xz --directory ffmpeg --strip-components=1 && rm -f ffmpeg-download.tar.xz && chmod -R og+x ffmpeg; fi"
]
},
"config": {
"secure-http": false

This comment has been minimized.

@kabel

kabel Apr 27, 2016

Contributor

Not that this is related to this pull request, but I don't think pear is actually used in the project. I would vote to remove it from composer, rather than add this config setting (dependency audit).

protected function createQuery()
{
return new Doctrine_Query();
}
}

This comment has been minimized.

@kabel

kabel Apr 27, 2016

Contributor

EOF

This comment has been minimized.

@mfairchild365

mfairchild365 Apr 28, 2016

Contributor

I didn't touch that line. :p

@@ -51,7 +51,7 @@ public function filterInputOptions()
*
* @see UNL/MediaHub/UNL_MediaHub_List#setOrderBy()
*/
function setOrderBy(Doctrine_Query &$query)
function setOrderBy(Doctrine_Query_Abstract &$query)

This comment has been minimized.

@kabel

kabel Apr 27, 2016

Contributor

Please remove the & style of param passing (php4). Objects should always come by reference.

@@ -8,7 +8,7 @@ function __construct(UNL_MediaHub_Subscription $subscription)
$this->subscription = $subscription;
}
function apply(Doctrine_Query &$query)
function apply(Doctrine_Query_Abstract &$query)

This comment has been minimized.

@kabel

kabel Apr 27, 2016

Contributor

Same thing about &

@@ -8,7 +8,7 @@ function __construct(UNL_MediaHub_User $user)
$this->user = $user;
}
function apply(Doctrine_Query &$query)
function apply(Doctrine_Query_Abstract &$query)

This comment has been minimized.

@kabel

kabel Apr 27, 2016

Contributor

Same thing about &

@@ -10,7 +10,7 @@ function __construct(UNL_MediaHub_User $user, $mediaId)
$this->mediaId = $mediaId;
}
function apply(Doctrine_Query &$query)
function apply(Doctrine_Query_Abstract &$query)

This comment has been minimized.

@kabel

kabel Apr 27, 2016

Contributor

Same thing about &

@@ -2,7 +2,7 @@
class UNL_MediaHub_FeedList_Filter_Popular implements UNL_MediaHub_Filter
{
function apply(Doctrine_Query &$query)
function apply(Doctrine_Query_Abstract &$query)

This comment has been minimized.

@kabel

kabel Apr 27, 2016

Contributor

Same thing about &

@@ -8,7 +8,7 @@ function __construct($id)
$this->id = $id;
}
function apply(Doctrine_Query &$query)
function apply(Doctrine_Query_Abstract &$query)

This comment has been minimized.

@kabel

kabel Apr 27, 2016

Contributor

Same thing about &

@kabel

This comment has been minimized.

Copy link
Contributor

kabel commented Apr 27, 2016

Coding standards in general. If you change a line, please do you best to ensure that line follows the PSR-2 standard as much as possible (like putting public visibility on functions that are missing it).

@kabel kabel merged commit d6a4d62 into unl:4.0_template Apr 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment