Skip to content

Fix for CacheProvider serious performance issue #125

Merged
merged 3 commits into from Apr 14, 2012

6 participants

@sobstel
sobstel commented Mar 28, 2012

Prior implementation for CacheProvider was seriously flawed from performance point of view as each fetch() call results in 2 additional cache storage calls due to $this->getNamespaceId(), which always checks for $namespaceCacheKey (and fetches it if exists, which will be the case for 2nd and next calls).

It is big overhead (3 calls instead just one for each fetch), and actually can be easily solved by memoizing namespace version value.

sobstel and others added some commits Mar 28, 2012
@sobstel sobstel CacheProvider: memoize namespace version value.
Prior implementation for CacheProvider was seriously flawed
from performance point of view as each fetch() call results
in 2 additional cache storage calls due to $this->getNamespaceId(),
which always checks for $namespaceCacheKey (and fetches it if
exists, which will be the case for 2nd and next calls).
7bad040
@florin-innobyte florin-innobyte CacheProvider: avoid double call to fetch $namespaceVersion. 6d05190
@beberlei beberlei and 2 others commented on an outdated diff Mar 30, 2012
lib/Doctrine/Common/Cache/CacheProvider.php
+ {
+ return sprintf(self::DOCTRINE_NAMESPACE_CACHEKEY, $this->namespace);
+ }
+
+ /**
+ * Namespace version
+ *
+ * @return string $namespaceVersion
+ */
+ private function getNamespaceVersion()
+ {
+ if (NULL === $this->namespaceVersion)
+ {
+ $namespaceCacheKey = $this->getNamespaceCacheKey();
+ $namespaceVersion = $this->doFetch($namespaceCacheKey);
+ if (false === $namespaceVersion) {
@beberlei
Doctrine member
beberlei added a note Mar 30, 2012

can you guarantee this for all cache drivers? or is false == the better condition here? Since its definately not 1 its a sufficient check.

@dlsniper
dlsniper added a note Mar 30, 2012

Since @sobstel did the change on my recommendation I'd like to jump in on this one.

As mentioned on #122, "if the documentation here, https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Cache/CacheProvider.php#L144 , is right, then fetch will return FALSE in case the value is not found so I don't see a point in doing a trip to the caching server, ask if it has the value in it's cache, if it has fetch it, if not then" we should change the function documentation as it's wrong.

@beberlei what's your input on this?

@dlsniper
dlsniper added a note Mar 30, 2012

I've also created some tests in #127 which should help with that but I couldn't test all the cache drivers. Memcache(d), APC, Array pass this condition, Zend should too since it's based on APC and I'm clueless about the rest.

@sobstel
sobstel added a note Mar 31, 2012

"== condition" will also work as we're expecting integer here. We could even do ($namespaceVersion < 1) condition. But as doFetch($id) is supposed to return false, it should be ok as well. And if it's not case for some cache driver, then it should be fixed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Mar 30, 2012
lib/Doctrine/Common/Cache/CacheProvider.php
@@ -117,10 +122,11 @@ public function flushAll()
*/
public function deleteAll()
{
- $namespaceCacheKey = sprintf(self::DOCTRINE_NAMESPACE_CACHEKEY, $this->namespace);
- $namespaceVersion = ($this->doContains($namespaceCacheKey)) ? $this->doFetch($namespaceCacheKey) : 1;
+ $namespaceCacheKey = $this->getNamespaceCacheKey();
+ $namespaceVersion = $this->getNamespaceVersion() + 1;
+ $this->namespaceVersion = $namespaceVersion;
@guilhermeblanco
Doctrine member

Add a line space before this line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco and 1 other commented on an outdated diff Mar 30, 2012
lib/Doctrine/Common/Cache/CacheProvider.php
+ *
+ * @return string $namespaceCacheKey
+ */
+ private function getNamespaceCacheKey()
+ {
+ return sprintf(self::DOCTRINE_NAMESPACE_CACHEKEY, $this->namespace);
+ }
+
+ /**
+ * Namespace version
+ *
+ * @return string $namespaceVersion
+ */
+ private function getNamespaceVersion()
+ {
+ if (NULL === $this->namespaceVersion)
@guilhermeblanco
Doctrine member

Use "null" (lowercased) here

@stof
Doctrine member
stof added a note Mar 30, 2012

and the curly brace should be on the same line than the if

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on the diff Mar 30, 2012
lib/Doctrine/Common/Cache/CacheProvider.php
return sprintf('%s[%s][%s]', $this->namespace, $id, $namespaceVersion);
}
/**
+ * Namespace cache key
+ *
+ * @return string $namespaceCacheKey
+ */
+ private function getNamespaceCacheKey()
+ {
+ return sprintf(self::DOCTRINE_NAMESPACE_CACHEKEY, $this->namespace);
+ }
+
+ /**
+ * Namespace version
+ *
+ * @return string $namespaceVersion
+ */
+ private function getNamespaceVersion()
@guilhermeblanco
Doctrine member

Ok, this whole code needs to be refactored.
It breaks the rule #1 of Object Calisthenics: "Only one level of indentation per method".
You can rewrite this one using early returns

private function getNamespaceVersion()
{
    if (null !== $this->namespaceVersion) {
        return $this->namespaceVersion;
    }

    $namespaceCacheKey = $this->getNamespaceCacheKey();
    $namespaceVersion  = $this->doFetch($namespaceCacheKey);

    if (false === $namespaceVersion) {
        $namespaceVersion = 1;

        $this->doSave($namespaceCacheKey, $namespaceVersion);
    }

    $this->namespaceVersion = $namespaceVersion;

    return $this->namespaceVersion;
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Mar 31, 2012
lib/Doctrine/Common/Cache/CacheProvider.php
@@ -40,6 +40,11 @@
private $namespace = '';
/**
+ * @var string The namespace version
+ */
+ private $namespaceVersion = NULL;
@stof
Doctrine member
stof added a note Mar 31, 2012

remove the = null. It is useless as it is already the default in PHP when omitting it

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

Hi,

What's needed in order for this to be merged?

Thanks.

@stof
Doctrine member
stof commented Apr 13, 2012
@guilhermeblanco guilhermeblanco merged commit 4f78640 into doctrine:master Apr 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.