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

Add an array implementation of cache and use it if we are not debugging #13269

Merged
merged 1 commit into from Feb 16, 2015

Conversation

Projects
None yet
6 participants
@nickvergessen
Copy link
Contributor

nickvergessen commented Jan 12, 2015

@@ -41,6 +41,8 @@ function create($prefix = '') {
return new Redis($prefix);
} elseif (Memcached::isAvailable()) {
return new Memcached($prefix);
} elseif (!defined('DEBUG')) {

This comment has been minimized.

@LukasReschke

LukasReschke Jan 12, 2015

Member

Generally speaking I think it might make sense if debug mode is enabled to cache nothing at all. When developing things and you have APCU things like the autoloader are getting annoying very soon otherwise. (which caches some things here)

This comment has been minimized.

@LukasReschke

LukasReschke Jan 12, 2015

Member

Related to #13227 as well

This comment has been minimized.

@nickvergessen

nickvergessen Jan 12, 2015

Author Contributor

It's a different issue thou?

This comment has been minimized.

@LukasReschke

LukasReschke Jan 12, 2015

Member

Well I wonder why we should not return the ArrayCache in case the debug mode is enabled but will return all other caches.

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 12, 2015

Great initiative – thank you.

Some unit tests for this would make this even more awesome ;)

@nickvergessen

This comment has been minimized.

Copy link
Contributor Author

nickvergessen commented Jan 12, 2015

@LukasReschke it has test coverage because it extends the Cache testcase ;)

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 12, 2015

@LukasReschke it has test coverage because it extends the Cache testcase ;)

Silly me insert random facepalm meme

@DeepDiver1975 DeepDiver1975 added this to the 8.1-next milestone Jan 12, 2015

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 12, 2015

Let's have a look at this after 8.0 release - not critical at the moment from my pov

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 12, 2015

Just for the general picture: This fixes issues such as #13237 (comment) which will reduce the search time (and a lot of other actions) to a lot of time when no cache is available. This often means a reduced search time of 10 seconds and more.

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Jan 12, 2015

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

@Xenopathic

This comment has been minimized.

Copy link
Member

Xenopathic commented Jan 12, 2015

👍

@nickvergessen

This comment has been minimized.

Copy link
Contributor Author

nickvergessen commented Feb 9, 2015

Since 8.0 is out, would be nice to have this merged, before the rework is started.

@Xenopathic

This comment has been minimized.

Copy link
Member

Xenopathic commented Feb 9, 2015

One thing, remove the TTL code. The array object will only exist for a single request, so TTLs won't have any effect other than slight performance degredation.

@nickvergessen

This comment has been minimized.

Copy link
Contributor Author

nickvergessen commented Feb 9, 2015

but then the tests for that won't work anymore and we need to state that explicit?

@nickvergessen nickvergessen force-pushed the issue/13211-cache-array-implementation branch from dc16016 to 40ea3c1 Feb 9, 2015

@nickvergessen

This comment has been minimized.

Copy link
Contributor Author

nickvergessen commented Feb 9, 2015

Seems like TTL is not tested... I removed it now

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Feb 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/9050/
Test PASSed.

@Xenopathic

This comment has been minimized.

Copy link
Member

Xenopathic commented Feb 9, 2015

👍

foreach ($this->cachedData as $key => $value) {
if ($prefix === '' || strpos($key, $prefix) === 0) {
$this->remove($key);
}

This comment has been minimized.

@Xenopathic

Xenopathic Feb 11, 2015

Member

What about simply using $this->cachedData = array() here instead of the loop?

This comment has been minimized.

@nickvergessen

nickvergessen Feb 13, 2015

Author Contributor

Well that would ignore the prefix. But we can do that for the $prefix === '' case

This comment has been minimized.

@Xenopathic

Xenopathic Feb 13, 2015

Member

Ah, good point.

@nickvergessen nickvergessen force-pushed the issue/13211-cache-array-implementation branch from 40ea3c1 to 0f3bda4 Feb 13, 2015

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Feb 13, 2015

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

@nickvergessen

This comment has been minimized.

Copy link
Contributor Author

nickvergessen commented Feb 13, 2015

Second +1? @LukasReschke

@nickvergessen nickvergessen force-pushed the issue/13211-cache-array-implementation branch from 0f3bda4 to 8848b5f Feb 16, 2015

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Feb 16, 2015

As discussed: 👍

THX!

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Feb 16, 2015

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

nickvergessen added a commit that referenced this pull request Feb 16, 2015

Merge pull request #13269 from owncloud/issue/13211-cache-array-imple…
…mentation

Add an array implementation of cache and use it if we are not debugging

@nickvergessen nickvergessen merged commit 8eb804b into master Feb 16, 2015

1 check passed

default Merged build finished.
Details

@nickvergessen nickvergessen deleted the issue/13211-cache-array-implementation branch Feb 16, 2015

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Feb 16, 2015

The inspection completed: No issues found

1 similar comment
@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Feb 16, 2015

The inspection completed: No issues found

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