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

Use memcache for findBinaryPath #13192

Merged
merged 2 commits into from Jan 9, 2015

Conversation

Projects
None yet
8 participants
@Xenopathic
Copy link
Member

Xenopathic commented Jan 9, 2015

Finding executables is slow, so I memcache'd it. (not expecting this for oC 8, just putting it here while I'm doing performance stuff)

Also introduces a Null memcache backend, that is returned from the factory instead of null, which pretends to do caching but really does nothing. It means we no longer need to explicitly check if a memcache is available every time we use it. It's like /dev/null!

cc @PVince81 @nickvergessen @icewind1991

Robin McCorkell added some commits Jan 9, 2015

Robin McCorkell
Memcache binary executable searching
It's slow, this makes it fast!
@@ -882,13 +882,19 @@ public static function is_function_enabled($function_name) {
* @return null|string
*/
public static function findBinaryPath($program) {
$memcache = \OC::$server->getMemCacheFactory()->create('findBinaryPath');

This comment has been minimized.

@LukasReschke

LukasReschke Jan 9, 2015

Member

In this context here since it is static why not just using a variable to cache it?

This comment has been minimized.

@LukasReschke

LukasReschke Jan 9, 2015

Member

Ahh - stupid me - that would not help much for future requests.

This comment has been minimized.

@Xenopathic

Xenopathic Jan 9, 2015

Author Member

It'd be nice to cache the ->create(), but I wanted to avoid more static variables. Perhaps the caching should be done in the factory (creating instances and returning them), but TBH a Memcache backend is very lightweight as it is, so I don't think it matters 😄

@LukasReschke LukasReschke added this to the 8.1-next milestone Jan 9, 2015

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 9, 2015

Makes sense 👍

@@ -42,7 +42,7 @@ function create($prefix = '') {
} elseif (Memcached::isAvailable()) {
return new Memcached($prefix);
} else {
return null;
return new Null($prefix);

This comment has been minimized.

@nickvergessen

nickvergessen Jan 9, 2015

Contributor

I wonder whether we should add an array implementation, that at least would cache the stuff for this call.

This comment has been minimized.

@Xenopathic

Xenopathic Jan 9, 2015

Author Member

Good idea. Want me to rebase my commit and implement that instead of Null?

This comment has been minimized.

@nickvergessen

nickvergessen Jan 9, 2015

Contributor

Just add it additionally (keep Null, might be useful elsewhere, or even use Null when defined('DEBUG') and Array otherwise)

@icewind1991

This comment has been minimized.

Copy link
Member

icewind1991 commented Jan 9, 2015

👍 looks good

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Jan 9, 2015

The inspection completed: 1 new issues, 7 updated code elements

@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/6164/
👍 Test PASSed. 👍

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 9, 2015

@Xenopathic can we have a performance comparison on this? THX

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 9, 2015

Test script:

<?php

session_write_close();
require_once('./lib/base.php');


for ($i = 1; $i <= 1000; $i++) {
    OC_Helper::findBinaryPath('sendmail');
}

1 is master, 2 is this branch - pretty impressive:

screen shot 2015-01-09 at 16 57 16
screen shot 2015-01-09 at 16 59 35
screen shot 2015-01-09 at 16 57 11

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 9, 2015

Thanks @LukasReschke - yes - impressive - using which memcache implementation?

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 9, 2015

@DeepDiver1975 APCu on 5.6

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 9, 2015

Okay - I'm all in for OC8 @karlitschek

@DeepDiver1975

This comment has been minimized.

Copy link
Member

DeepDiver1975 commented Jan 9, 2015

👍

1 similar comment
@karlitschek

This comment has been minimized.

Copy link
Member

karlitschek commented Jan 9, 2015

👍

@LukasReschke LukasReschke modified the milestones: 8.0-current, 8.1-next Jan 9, 2015

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 9, 2015

In case somebody else wonders how you can generate those nice call graphs and compare different runs: I use https://blackfire.io from SensioLabs (the guys behind Symfony). Can only highly recommend it :-)

@Xenopathic

This comment has been minimized.

Copy link
Member Author

Xenopathic commented Jan 9, 2015

@nickvergessen If it's OK with you, can this PR go ahead as it is at the moment? An array memcache backend can be introduced in a further PR if necessary.

@nickvergessen

This comment has been minimized.

Copy link
Contributor

nickvergessen commented Jan 9, 2015

@Xenopathic yes, fine by me Null just behaves like the current implementation, so shouldn't be worse

DeepDiver1975 added a commit that referenced this pull request Jan 9, 2015

@DeepDiver1975 DeepDiver1975 merged commit d4355ca into master Jan 9, 2015

1 check passed

default Merged build finished.
Details

@DeepDiver1975 DeepDiver1975 deleted the cache_binary_path branch Jan 9, 2015

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 9, 2015

I created a ticket for the array key cache: #13211

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