Skip to content

[Cache] Added PhpFilesAdapter #18832

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

Closed
wants to merge 7 commits into from
Closed

Conversation

trakos
Copy link
Contributor

@trakos trakos commented May 21, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

This PR adds PhpFilesAdapter, that works almost in the same way as FilesystemAdapter, but saving creates a .php file that is included on fetch (allowing opcache to kick in).

I later realized there is a similar PR: #18823 , but this one is not read-only and simpler (allows to use the regular cache interface), so might still be useful anyway.

At my company we use a similar approach to cache annotations, validations, doctrine metadata and queries. It's useful, because that way we can warm up cache during build. It performs much better than regular filesystem cache.

Writing files might actually be slightly slower than for regular cache, so it's meant for data that rarely changes.

@GromNaN
Copy link
Member

GromNaN commented May 21, 2016

Depending on the OPCache configuration, the cache can persist when the file is modified.
You may have to invalidate the cache with opcache_invalidate when the file is written.

@trakos
Copy link
Contributor Author

trakos commented May 22, 2016

Thanks for the advice. I've added calls to opcache_invalidate and apc_compile_file/apc_delete_file when those functions exists.

I had some problems with HHVM due to facebook/hhvm#4797 . On HHVM once php file is included during the request it won't be reloaded no matter what (in this particular request).

To avoid this when on HHVM I'm using file modification time to save cache item version number that I increment on save. It's also kept in file contents. When this numbers are different (checked using filemtime andinclude), I fallback to file_get_contents to fetch fresh data. The main drawback is that I can't delete cache items for HHVM, and I save empty expired value instead. Still, this cache is intended only for data that very rarely changes, so I don't think it matters.

@xabbuh
Copy link
Member

xabbuh commented May 23, 2016

I am not sure if this is a good idea actually. If I understand the code correctly, users of this adapter will have to be extra carefully to not cache arbitrary data as they will otherwise be executed when the cache is loaded which could make applications prone to code injection.

@trakos
Copy link
Contributor Author

trakos commented May 23, 2016

Actually, no, all data can be safely used with this adapter.

I just did a quick search and symfony already does similar thing for dumping translations:

return "<?php\n\nreturn ".var_export($messages->all($domain), true).";\n";

Anyway, here is how this works in this adapter: the data gets exported using var_export and serialize. PHP docs state that var_export Outputs or returns a parsable string representation of a variable. So doing var_export on a serialized string will always result in a string that eval'd will evaluate to the serialized string.

For instance, let's take this nasty string:

$data = '"\'; echo \'a\';"' . PHP_EOL . "\0" . '\n\r';

The cache adapter will create this file:

<?php return unserialize('s:21:""\'; echo \'a\';"
' . "\0" . '\\n\\r";');

As you can see, it can handle everything fine, including null bytes, new lines and stuff.

Of course it works just as fine for arrays, objects etc., for example this:

$data = new \DateTime();

Generates:

<?php return unserialize('O:8:"DateTime":3:{s:4:"date";s:26:"2016-05-23 08:59:28.000000";s:13:"timezone_type";i:3;s:8:"timezone";s:3:"UTC";}');

@nicolas-grekas
Copy link
Member

This is interesting and would need to be benched against other approaches (#18823 esp.)


use Symfony\Component\Cache\Exception\InvalidArgumentException;

class FilesCacheHelper
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not an abstract like you did previously?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to move some logic away (I couldn't just delete files for hhvm), and I just thought it's going to be more readable this way. Plus you could add tests to it this way if it will grow larger.

I can move it back to abstract if you prefer, I don't mind.

@nicolas-grekas
Copy link
Member

Yep, append-only for hhvm and native PHP: given the way opcache works (see http://jpauli.github.io/2015/03/05/opcache.html#understanding-the-opcache-memory-consumption), doing rewrites creates wasted memory, which we shouldn't. If one want rewrites, one should use another adapter IMHO.

@ttrakos
Copy link

ttrakos commented May 23, 2016

OK, I can modify it. Should it sent an exception if data is already there? Should it also sent exception when lifetime is not infinite?

I think clear method should still be supported, just to make clear-cache and such work, right?

@nicolas-grekas
Copy link
Member

It should return false: PSR-6 doesn't allow exceptions for error reporting.
For clear cache, you'd need a strategy the might look like #18716 (we renamed nonce to version in a recent PR)

@trakos
Copy link
Contributor Author

trakos commented May 24, 2016

I made the adapter append-only.

I did a quick test to compare my adapter with OpCacheAdapter from other PR.

For my test I disabled revalidating opcache entirely (with revalidation enabled PhpFilesAdapter gets slower). I saved some values, each of them consisting of an array with 100 strings, 50 characters length each. I did my tests for PHP 7.0.6 on windows (with SSD, but that shouldn't matter for opcache).

Here's saving 5000 values, then read randomly 10000 times:

 Symfony\Component\Cache\Adapter\FilesystemAdapter: 6.8619,   88 megabytes peak memory usage
   Symfony\Component\Cache\Adapter\PhpFilesAdapter: 8.4227,   88 megabytes peak memory usage
    Symfony\Component\Cache\Adapter\OpCacheAdapter: 2.0824,  286 megabytes peak memory usage

For next tests I was reloading it until the response time settled down:

Here's results for 10 values read randomly 10000 times:

 Symfony\Component\Cache\Adapter\FilesystemAdapter: 0.8210
   Symfony\Component\Cache\Adapter\PhpFilesAdapter: 0.1625
    Symfony\Component\Cache\Adapter\OpCacheAdapter: 0.0115

Here's results for 5000 values read randomly 10000 times:

 Symfony\Component\Cache\Adapter\FilesystemAdapter: 1.7495
   Symfony\Component\Cache\Adapter\PhpFilesAdapter: 0.1820
    Symfony\Component\Cache\Adapter\OpCacheAdapter: 0.0125

OpCacheAdapter is slightly faster, but requires saving everything at once during initialization (no appends, regular save not supported).

@nicolas-grekas
Copy link
Member

Interesting. Can you please add apcu to the tested adapters, try on php 5.5&5.6, and/or share the test script?

@trakos
Copy link
Contributor Author

trakos commented May 25, 2016

Here are results for php 7 including apcu (I've changed saving test a bit to better show the difference in required memory):

Storing 5000 items and fetching them randomly 10000 times (no warmup):

 Symfony\Component\Cache\Adapter\FilesystemAdapter: 7.4497,    2 megabytes
   Symfony\Component\Cache\Adapter\PhpFilesAdapter: 1.5632,    2 megabytes
    Symfony\Component\Cache\Adapter\OpCacheAdapter: 1.7042,  286 megabytes
       Symfony\Component\Cache\Adapter\ApcuAdapter: 0.2520,    2 megabytes

Warmup....................

Fetching randomly 5000 items 10000 times (after warmup):

 Symfony\Component\Cache\Adapter\FilesystemAdapter: 1.4061,    2 megabytes
   Symfony\Component\Cache\Adapter\PhpFilesAdapter: 0.1710,    2 megabytes
    Symfony\Component\Cache\Adapter\OpCacheAdapter: 0.0140,    2 megabytes
       Symfony\Component\Cache\Adapter\ApcuAdapter: 0.1180,    2 megabytes

Storing 10 items and fetching them randomly 10000 times (no warmup):

 Symfony\Component\Cache\Adapter\FilesystemAdapter: 0.7901,    2 megabytes
   Symfony\Component\Cache\Adapter\PhpFilesAdapter: 0.1640,    2 megabytes
    Symfony\Component\Cache\Adapter\OpCacheAdapter: 0.0200,    2 megabytes
       Symfony\Component\Cache\Adapter\ApcuAdapter: 0.1150,    2 megabytes

Warmup....................

Fetching randomly 10 items 10000 times (after warmup):

 Symfony\Component\Cache\Adapter\FilesystemAdapter: 0.7881,    2 megabytes
   Symfony\Component\Cache\Adapter\PhpFilesAdapter: 0.1610,    2 megabytes
    Symfony\Component\Cache\Adapter\OpCacheAdapter: 0.0130,    2 megabytes
       Symfony\Component\Cache\Adapter\ApcuAdapter: 0.1130,    2 megabytes

I can try with different php versions later this week. Here are files I've tested with:
PHP file on server: https://gist.github.com/trakos/2395d896eb7ad51cac05c22d169b5ef4
.sh script fetching data with curl: https://gist.github.com/trakos/a5dd8482ff8239eb51d2d5f1ec39a4f4

As for settings, I've disabled opcache verification and increased memory for both apcu and opcache.

@nicolas-grekas
Copy link
Member

Looks promising, I think we could provide this adapter as an alternative to the apcu adapter.
I reworked the patch here: master...nicolas-grekas:cache-php-files
WDYT about it?

@ttrakos
Copy link

ttrakos commented May 25, 2016

Thanks for clearing the code out.
You've dropped hhvm support - do you think it's not worth it?
Also, you're leaving it to user to not overwrite existing values, right?

@nicolas-grekas
Copy link
Member

You've dropped hhvm support - do you think it's not worth it?

it's intentional, because per your benchs, hhvm should use apc for about the same perf benefit

you're leaving it to user to not overwrite existing values

good catch, I'm going to fix that

@nicolas-grekas
Copy link
Member

you're leaving it to user to not overwrite existing values

Hum in fact I'm not sure I understand what you mean? The user can still overwrite values safely, isn't it?
If you mean append-only vs rewrite support, then it a yes, it's now up to the user to use the adapter in append mostly scenarios. More flexible imho, less error prone...

@ttrakos
Copy link

ttrakos commented May 25, 2016

If I read this right, deleting key returns true, but does not call opcache_invalidate, so depending on settings it might mean value will still be returned on fetch. Do I understand this correctly?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 25, 2016

I though the same, then I tested the real behavior, and deleted files are always taken into account (include fails) whatever the ini settings.

@nicolas-grekas
Copy link
Member

Patch integrated into #18823
Thank you @trakos @ttrakos

nicolas-grekas added a commit that referenced this pull request Jun 6, 2016
This PR was merged into the 3.2-dev branch.

Discussion
----------

[Cache] Added PhpFilesAdapter

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This is taking over #18832.
With a warm cache I get these numbers consistently (PhpArrayAdapter being the implem in #18823 ):
```
Fetching randomly 5000 items 10000 times:

 Symfony\Component\Cache\Adapter\FilesystemAdapter: 0.1367,    2 megabytes
   Symfony\Component\Cache\Adapter\PhpArrayAdapter: 0.0071,    2 megabytes
   Symfony\Component\Cache\Adapter\PhpFilesAdapter: 0.0389,    2 megabytes
       Symfony\Component\Cache\Adapter\ApcuAdapter: 0.0361,    2 megabytes
```

This means that the PhpArrayAdapter should be used first, then ApcuAdapter preferred over PhpFilesAdapter, then FilesystemAdapter. This is what AbstractAdapter does here.

Also note that to get the cache working, one should stay within the limits defined by the following ini settings:
- memory_limit
- apc.shm_size
- opcache.memory_consumption
- opcache.interned_strings_buffer
- opcache.max_accelerated_files

Commits
-------

8983e83 [Cache] Optimize & wire PhpFilesAdapter
14bcd79 [Cache] Added PhpFilesAdapter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants