Skip to content
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 apcu_* functions instead of apc_* functions if available #274

Closed
layanto opened this issue Dec 8, 2015 · 21 comments
Closed

Use apcu_* functions instead of apc_* functions if available #274

layanto opened this issue Dec 8, 2015 · 21 comments

Comments

@layanto
Copy link

layanto commented Dec 8, 2015

This still rely on APC backward compatibility of APCu where all apc_* functions internally call apcu_* functions. APC backward compatibility is included by default for APCu on PHP5.5 and PHP5.6. But for PHP7, APC backward compatibility is not included by default (and still not working on Windows). Any chance to update the APC driver to check for existence of apcu_fetch function and if so use the apcu_* functions with fallback to apc_* functions?
Similar to
https://github.com/illuminate/cache/blob/master/ApcWrapper.php

See #256

@limenet
Copy link
Contributor

limenet commented Dec 12, 2015

Definitely a +1, would be really great to be able to [continue to] use APC(u) under PHP 7.0!

@tedivm
Copy link
Member

tedivm commented Dec 14, 2015

I will do this, but it's lower on the queue right now because Travis-CI does not have PECL support for PHP7 yet.

@limenet
Copy link
Contributor

limenet commented Dec 14, 2015

@tedivm I recently tried to work on a PR for this issue and as a first step tried to get Travis-CI running and pass the tests. After a few attempts I got phpunit on Travis-CI to run the tests but then I got a segfault. After recreating that on a Vagrant box it looks like the segfault is due to Redis. And since my experience with Redis is zero (and I didn't feel like using gdb or similar) I decided to stop trying for now.

Anyhow, maybe you could use some code from my fork for Travis-CI+PHP 7+PECL, see also the latest build. Just wanted to let you know, maybe it helps.

tedivm added a commit that referenced this issue Dec 14, 2015
This should solve ##274.
@tedivm
Copy link
Member

tedivm commented Dec 14, 2015

@limenet - I actually got the APCU stuff working on my test box. Funny enough not all versions of APCU have the APCUItorator class, which is really important for a number of things, but the newer versions do have it.

Anyways, its up in the feature branch for v1.0.0 if you want to give it a shot.

@limenet
Copy link
Contributor

limenet commented Dec 14, 2015

@tedvim thanks, I saw that branch... after I tried to get the master branch to work.

I installed APCu from Ondrej's PPA on my Ubuntu 14.04 PHP 7 test sever (Digitalocean$5 droplet) and used the composite driver with apc, file system, sqlite (in that order). Execution times (for cache hits) increased from less than 100 ms to more than 1 second.
On the PHP 5.6 sever which is identically set up (except for the PHP version), execution times with apc were better than without apc (and around 150 ms), as expected.

Cache sizes were set to 64M on both severs and a Feb hundred KB were actually stored in cache. The cache was fresh and not fragmented.

Do you have any idea why that's the case?

Edit: I tested it on Saturday.

@tedivm
Copy link
Member

tedivm commented Dec 14, 2015

Can you clarify what you're asking? Are you stating that the same versions of the code are showing better performance with APC on PHP 5.6 than APCu on PHP7?

@limenet
Copy link
Contributor

limenet commented Dec 14, 2015

Exactly, at least that's what it looked like, but it sounds very strange to me. I'll test it again and let you know.

@tedivm
Copy link
Member

tedivm commented Dec 14, 2015

Okay- I don't know of anything in the code that would treat them differently, so I'm wondering if it's an upstream thing. APCU has only supported PHP7 for three weeks now, so it may be buggy.

@limenet
Copy link
Contributor

limenet commented Dec 14, 2015

Due to 40ee61f my testing will take a bit longer than I anticipated, but I'll definitely look into it!

I also suspect an upstream problem, since PHP 7 itself should be faster than PHP 5.6.

@limenet
Copy link
Contributor

limenet commented Dec 14, 2015

I just tested it again and it looks like either the server or me had a strange day when I tested it last time. The load times are now where they should be. Sorry for the trouble!


EDIT: I know I'm contradicting myself and probably annoying, too. I just tested it again and now the load times are high again. (4.8 seconds)

@limenet
Copy link
Contributor

limenet commented Dec 14, 2015

@tedivm do you happen to know the reason for this error message? (v1.0.0-dev)

E_WARNING: APCuIterator::__construct() expects parameter 2 to be integer, string given
in Stash/Driver/Apc.php on line 124

@limenet
Copy link
Contributor

limenet commented Dec 14, 2015

I just tested the fix and it seems to be fully working.

@limenet
Copy link
Contributor

limenet commented Dec 14, 2015

Sorry for spamming this morning!

Thanks to apc.php (from /usr/share/doc/php-apcu/) I first noticed APCu wasn't even being hit as a cache - thanks to an erroneous config setting on my part. After fixing that, the cache reported a 100% miss rate. To test whether APCu was even in a working state, I tried example 1 from http://php.net/manual/en/function.apcu-add.php - and it worked. Also, $this->apcu is set correctly.

As far as I can tell Stash\Driver\Apc::storeData() stores the data (apcu_store returns true) but Stash\Driver\Apc::getData() always returns false i.e. doesn't store the data. (apc.php also doesn't list any cache entries besides the one from the php.net example).

Next I (accidentally) hit the reload button on apc.php when the request was still loading and it looks like there are entries in the cache during the request. They are cleared, however, once the request has finished.
As an attempt, I bypassed Pool::deleteItem() and Pool::clear() to make sure I wasn't calling any of these functions // with wrong arguments. No luck there.

(Side question: why is APCu caching set to a default TTL of 30 seconds?)

Does anyone have an idea what is wrong or where I could try to have a look at next?


In case it helps:

image

@tedivm
Copy link
Member

tedivm commented Dec 14, 2015

How are you testing? With a web server or through the command line?

@limenet
Copy link
Contributor

limenet commented Dec 14, 2015

I'm testing using Apache 2.4, not via the CLI.

@limenet
Copy link
Contributor

limenet commented Dec 15, 2015

I created a quick testcase https://gist.github.com/limenet/a6f9448d084af1444ab6 . It sets up an APC driver, stores some data using Stash and then the same data using apcu_* methods. Since the v1.0 docs are still missing, I'm not sure whether my logic is correct.

What do you think, should this script work as expected (i.e. on first load, generate and store data and on subsequent loads report a cache hit)?

My output on subsequent loads is (again via web server; apcu isn't enabled for cli by default):

MISS stash
HIT apcu

float(0.964035987854)

@limenet
Copy link
Contributor

limenet commented Dec 15, 2015

And after having a look at the test cases I found my mistake:
While with 0.x $item->set($data) worked, 1.0 requires $item->set($data)->save(). Correct? If so, I apologize for any trouble I may have caused, I'm really sorry!

@tedivm
Copy link
Member

tedivm commented Dec 15, 2015

To fit with PSR-6 you should save the item to the pool. So-

$item = $pool->getItem('key');
$item->set('value');
$pool->save($item);

@limenet
Copy link
Contributor

limenet commented Dec 16, 2015

Thanks @tedivm , I wasn't aware of that change! It works perfectly now.

@tedivm
Copy link
Member

tedivm commented Dec 16, 2015

Glad to hear it! Thanks for helping get this feature added!

@tedivm tedivm closed this as completed Dec 16, 2015
@limenet
Copy link
Contributor

limenet commented Dec 16, 2015

You're very welcome! Thank you for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants