Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ApcCache::doContains should use apc_exists() instead of apc_fetch(). It ... #114

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

slimacek commented Mar 12, 2012

...makes a huge difference in execution time if stored value is large.

@slimacek slimacek ApcCache::doContains should use apc_exists() instead of apc_fetch(). …
…It makes a huge difference if stored value is large.
f3b8570

@schmittjoh schmittjoh commented on the diff Mar 12, 2012

lib/Doctrine/Common/Cache/ApcCache.php
@@ -49,9 +49,7 @@ protected function doContains($id)
{
$found = false;
- apc_fetch($id, $found);
-
- return $found;
+ return apc_exists($id);
@schmittjoh

schmittjoh Mar 12, 2012

Member

This function was added in version "3.1.4" while "apc_fetch" exists since "3.0.0".

Do we make any guarantee what version of APC we support?

@hobodave

hobodave Mar 12, 2012

Contributor

I could find nothing in the documentation regarding versions of APC. Due to that, this should probably be a change made in 2.3 and not 2.2.x. Alternatively, one could add a constructor which uses function_exists to set a variable function to use for doContains().

@hobodave

hobodave Mar 12, 2012

Contributor

BTW this commit is messy, leaving the unused local variable $found in place.

Contributor

dlsniper commented Mar 26, 2012

If @slimacek is not around to finish this PR is it ok if I'll pick-up the work from where he stopped?

Member

stof commented Mar 26, 2012

@dlsniper sure it is. simply base your work on his branch so that you keep his authorship

Contributor

dlsniper commented Mar 26, 2012

@stof sorry but I'm clueless on how to do that, can you please send me a link or something that explains how to do it?
Can I simply add a new upstream then commit the changes to his branch? I've only did it so far with forking a certain repository after deleting my own fork but currently my fork has already a PR pending here....

Member

stof commented Mar 26, 2012

Closing in favor of #124 which fixes the remaining stuff

@stof stof closed this Mar 26, 2012

@beberlei beberlei added a commit that referenced this pull request Mar 30, 2012

@beberlei beberlei Merge pull request #124 from dlsniper/improve-apc-contains-114
Improve APC contains()  enhances #114
71c3bb7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment