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

Cache responses from the AppStore server #13212

Merged
merged 1 commit into from Jan 10, 2015

Conversation

Projects
None yet
7 participants
@LukasReschke
Copy link
Member

LukasReschke commented Jan 9, 2015

Otherwise every time the AppStore was opened a lot of connections to the AppStore server were made which resulted in a terrible performance.

This changeset will cache the response for a sensible time so that only the first request will be somewhat slow.

Performance changes:

  • Loading a category took previously more than 3 seconds on my machine. Now for every follow-up request it takes less than 200ms, resulting in a performance gain of 1950%
  • Loading the category list took previously about 750ms - now it takes 154ms, a total performance gain of 395%

Fixes #13209

@DeepDiver1975 @karlitschek FYI - please decide whether 8.0 or 8.1

@LukasReschke

This comment has been minimized.

Copy link
Member Author

LukasReschke commented Jan 9, 2015

Please note that to gain advantage of that change you need a memcache to be installed. A light-weigth and easy to install cache would be APCu.

@LukasReschke LukasReschke force-pushed the cache-appstore-response branch Jan 9, 2015

@LukasReschke LukasReschke force-pushed the cache-appstore-response branch Jan 9, 2015

@MorrisJobke

This comment has been minimized.

Copy link
Member

MorrisJobke commented Jan 9, 2015

Works 👍

@LukasReschke LukasReschke force-pushed the cache-appstore-response branch 2 times, most recently to 33f4962 Jan 9, 2015

@LukasReschke

This comment has been minimized.

Copy link
Member Author

LukasReschke commented Jan 9, 2015

@owncloud-bot Retest this please.

Cache responses from the AppStore server
Otherwise every time the AppStore was opened a lot of connections to the AppStore server were made which resulted in a terrible performance.

This changeset will cache the response for a sensible time so that only the first request will be somewhat slow.

Performance changes:
- Loading a category took previously more than 3 seconds on my machine. Now for every follow-up request it takes less than 200ms, resulting in a performance gain of 1950%
- Loading the category list took previously about 750ms - now it takes 154ms, a total performance gain of 395%

@LukasReschke LukasReschke force-pushed the cache-appstore-response branch from 33f4962 to b8b4df5 Jan 9, 2015

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 9, 2015

Awesome @LukasReschke - can we have the nice graphs for this as well? 🙊

@LukasReschke

This comment has been minimized.

Copy link
Member Author

LukasReschke commented Jan 9, 2015

can we have the nice graphs for this as well? 🙊

Absolutely.

Listing of available categories

screen shot 2015-01-09 at 20 17 23
categories

Viewing of single category

screen shot 2015-01-09 at 20 17 07
list

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Jan 9, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/6240/
👍 Test PASSed. 👍

@karlitschek

This comment has been minimized.

Copy link
Member

karlitschek commented Jan 9, 2015

nice 👍

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Jan 9, 2015

The inspection completed: 9 new issues

@DeepDiver1975 DeepDiver1975 added this to the 8.0-current milestone Jan 9, 2015

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 9, 2015

I vote for OC8 👍

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 9, 2015

@owncloud-bot retest this please

@@ -12,6 +12,9 @@
$result = OC_App::removeApp($appId);
if($result !== false) {
// FIXME: Clear the cache - move that into some sane helper method

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Jan 9, 2015

Member

can be done easily in the controller once be move these ajax/*.php files to AppFramework

parent::__construct($appName, $request);
$this->l10n = $l10n;
$this->config = $config;
$this->cache = $cache->create($appName);

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Jan 9, 2015

Member

generally speaking - injecting the cache might be easier with respect to unit testing -> 8.1

return array('apps' => $apps, 'status' => 'success');
$this->cache->set('listApps-'.$category, $apps, 300);
return ['apps' => $apps, 'status' => 'success'];

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Jan 9, 2015

Member

note to self: kill 'status'

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Jan 9, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/6251/

Build result: FAILURE

[...truncated 17 lines...]using GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git2331610545736736939.credentials # timeout=10 > git -c core.askpass=true fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse origin/pr/13212/merge^{commit} # timeout=10 > git branch -a --contains 3a51248ef93c848b3d5256708a796ea96d8203ca # timeout=10 > git rev-parse remotes/origin/pr/13212/merge^{commit} # timeout=10Checking out Revision 3a51248ef93c848b3d5256708a796ea96d8203ca (origin/pr/13212/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f 3a51248ef93c848b3d5256708a796ea96d8203ca > git rev-list 25252d9064fec7395b682f99da1c2e175ea2b9c0 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 1 second
💣 Test FAILed. 💣

@MorrisJobke

This comment has been minimized.

Copy link
Member

MorrisJobke commented Jan 9, 2015

Jenkins PR: #13222

@Xenopathic

This comment has been minimized.

Copy link
Member

Xenopathic commented Jan 9, 2015

Hooray, Jenkins in happy! 👍 for oC 8

MorrisJobke added a commit that referenced this pull request Jan 10, 2015

Merge pull request #13212 from owncloud/cache-appstore-response
Cache responses from the AppStore server

@MorrisJobke MorrisJobke merged commit ae34832 into master Jan 10, 2015

1 check passed

default Merged build finished.
Details

@MorrisJobke MorrisJobke deleted the cache-appstore-response branch Jan 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment