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

Introduce cache drivers #200

Merged
merged 4 commits into from
Jun 21, 2021
Merged

Introduce cache drivers #200

merged 4 commits into from
Jun 21, 2021

Conversation

lcobucci
Copy link
Collaborator

@lcobucci lcobucci commented Dec 18, 2019

Fixes #140

This provides a caching interface and implementations for the most common drivers (filesystem and APCu).
Users can create their own implementation of the interface to support PSR-6/PSR-16/anything else.

@lcobucci lcobucci requested a review from nikic December 18, 2019 18:14
@lcobucci lcobucci self-assigned this Dec 18, 2019
@lcobucci lcobucci added this to the 2.0.0 milestone Dec 18, 2019
@lcobucci lcobucci force-pushed the introduce-cache-drivers branch from 671b161 to 4e66622 Compare December 19, 2019 14:25
src/Caching/NullCache.php Outdated Show resolved Hide resolved
src/Caching/NullCache.php Outdated Show resolved Hide resolved
src/Caching/NullCache.php Outdated Show resolved Hide resolved
src/Caching/NullCache.php Outdated Show resolved Hide resolved
src/Caching/NullCache.php Outdated Show resolved Hide resolved
@lcobucci
Copy link
Collaborator Author

@GrahamCampbell thank you for putting the time to review but, as I've tried to explain in the commit message, these implementations are naive on purpose.
They're there to fulfil the needs of this lib and none of the multi-set/get/delete are actually used.
Main idea was to not add any external dependency while giving people the flexibility to plug in the library of their choice.

@GrahamCampbell
Copy link
Contributor

I really think it's best to provide nothing at all, than non-compliant implementations. The point of the PSR interop is not to have fewer dependencies, but to give the user a choice of what to use. Maybe just "suggest" a the psr cache implementation virtual package, and provide no implementations?

@lcobucci
Copy link
Collaborator Author

I get your point, though to not provide anything we'd have to break the previous API.

We can drop the null thing by reworking a few things but the simple file one must be kept for the bc reason.

So I defer this to @nikic

@nikic
Copy link
Owner

nikic commented Dec 26, 2019

I don't really have a problem with breaking API for the next major version, if necessary. Some thoughts on this PR:

  • As with pretty much anything that has "simple" in the library name (SOAP anyone?) the SimpleCache is not simple, and does much more than what this library actually needs. In particular, the only operations we need is a key-less fetch and store. As the SimpleCache interface (per @GrahamCampbell's comments) requires an involved implementation to satisfy all the constraints, might it make more sense to invert the relationship? That is, provide an adapter to SimpleCache, rather than the other way around.
  • Generally speaking, the use of file-caching is optimal for this purpose. In conjunction with opcache, it is the most efficient way to cache this data. I am wondering what the motivation for using a different caching approach is.
  • I suspect that the main motivation is cache invalidation, which is currently not handled by this library at all. It is assumed that the cache is invalidated / pre-populated on deploy, which may not always be the case. I'm not sure to what degree the migration to PSR-15 really addresses this problem.
  • The other main problem I see people encounter is that closures can't be cached -- I don't think this is addressed by this change at all though.

@GrahamCampbell
Copy link
Contributor

I am wondering what the motivation for using a different caching approach is.

I suppose the use case is php-fpm not having write permission to the disk?

@nikic
Copy link
Owner

nikic commented Dec 26, 2019

@GrahamCampbell If php-fpm does not have write permissions, that implies that the application must be using pre-deploy cache population, where the FastRoute cache should also be generated. Possibly this is not really easy to integrate right now though?

@lcobucci
Copy link
Collaborator Author

lcobucci commented Dec 30, 2019

I don't really have a problem with breaking API for the next major version, if necessary. Some thoughts on this PR:

  • As with pretty much anything that has "simple" in the library name (SOAP anyone?) the SimpleCache is not simple, and does much more than what this library actually needs. In particular, the only operations we need is a key-less fetch and store. As the SimpleCache interface (per @GrahamCampbell's comments) requires an involved implementation to satisfy all the constraints, might it make more sense to invert the relationship? That is, provide an adapter to SimpleCache, rather than the other way around.
  • Generally speaking, the use of file-caching is optimal for this purpose. In conjunction with opcache, it is the most efficient way to cache this data. I am wondering what the motivation for using a different caching approach is.

I understood from the original issue that people did want to use a different caching driver. We can definitely invert the relationship and provide an adapter for PSR-16.

I'd still provide the SimpleCache implementation (using our interface) because I do agree that it's better to rely on opcache for this. Renaming it might be good, though.

  • I suspect that the main motivation is cache invalidation, which is currently not handled by this library at all. It is assumed that the cache is invalidated / pre-populated on deploy, which may not always be the case. I'm not sure to what degree the migration to PSR-15 really addresses this problem.

Invalidation is tricky, using other drivers it gets worse. Although I'd treat this kind of thing as we do for metadata cache in Doctrine ORM: let people control it outside of the application life cycle. The problem is not about how to invalidate but rather when.

  • The other main problem I see people encounter is that closures can't be cached -- I don't think this is addressed by this change at all though.

This addresses #140 and not #141 indeed.


@nikic my main concern here is that I feel like you don't see value in supporting multiple cache drivers and am afraid that this might be a poor use of my time...

I'm wiling to rework this whole thing if you say it's something we want for v2 but if not let's just close it 👍

Lock file operations: 1 install, 20 updates, 0 removals
  - Upgrading doctrine/annotations (1.12.1 => 1.13.1)
  - Upgrading phpbench/phpbench (1.0.0 => 1.0.2)
  - Upgrading phpstan/phpstan (0.12.86 => 0.12.89)
  - Upgrading phpstan/phpstan-phpunit (0.12.18 => 0.12.19)
  - Upgrading phpunit/phpunit (9.5.4 => 9.5.5)
  - Locking psr/cache (1.0.1)
  - Upgrading sebastian/global-state (5.0.2 => 5.0.3)
  - Upgrading sebastian/type (2.3.1 => 2.3.2)
  - Upgrading slevomat/coding-standard (7.0.8 => 7.0.9)
  - Upgrading symfony/console (v5.2.7 => v5.3.0)
  - Upgrading symfony/filesystem (v5.2.7 => v5.3.0)
  - Upgrading symfony/finder (v5.2.4 => v5.3.0)
  - Upgrading symfony/options-resolver (v5.2.4 => v5.3.0)
  - Upgrading symfony/polyfill-ctype (v1.22.1 => v1.23.0)
  - Upgrading symfony/polyfill-intl-grapheme (v1.22.1 => v1.23.0)
  - Upgrading symfony/polyfill-intl-normalizer (v1.22.1 => v1.23.0)
  - Upgrading symfony/polyfill-mbstring (v1.22.1 => v1.23.0)
  - Upgrading symfony/polyfill-php73 (v1.22.1 => v1.23.0)
  - Upgrading symfony/polyfill-php80 (v1.22.1 => v1.23.0)
  - Upgrading symfony/process (v5.2.7 => v5.3.0)
  - Upgrading symfony/string (v5.2.6 => v5.3.0)
@lcobucci lcobucci force-pushed the introduce-cache-drivers branch 2 times, most recently from 81ac50f to 28c6046 Compare June 14, 2021 23:05
Allowing users to vary the implementation of route caching according to
their needs.

This makes the file system based cache a bit more robust by using
exclusive locking and preventing reads of partial files.

We're also adding an APCu-based implementation, so that we can compare
FastRoute to HackRouting.

More info:

- https://github.com/azjezz/hack-routing
- https://github.com/azjezz/benchmark-php-routing
lcobucci added 2 commits June 15, 2021 22:22
The APCu cache is meant to be used by users who know that APCu is
installed, as it requires explicit configuration of the caching driver.
@lcobucci lcobucci force-pushed the introduce-cache-drivers branch from 2e3166d to a53d563 Compare June 15, 2021 20:22
@lcobucci
Copy link
Collaborator Author

lcobucci commented Jun 15, 2021

@nikic I've reorganised my proposal according to the discussion.
APCu has been added so we can have https://github.com/azjezz/benchmark-php-routing comparing things in a better way (and will probably be removed).

The file-based cache now does exclusive locks and atomic renames to avoid a corrupted state. The interface is also straightforward now, allowing us to add new implementations (e.g.: supporting closures).

Would you have a look, please?

@lcobucci
Copy link
Collaborator Author

@nikic I decided to get this merged for now. Please review it whenever you have the time for it and we can improve things.

@lcobucci lcobucci merged commit 2230b97 into master Jun 21, 2021
@lcobucci lcobucci deleted the introduce-cache-drivers branch June 21, 2021 14:32
@designermonkey
Copy link

While I appreciate the work that has gone into this, and fully understanding that I am not a contributor to the project, I would like to raise two concerns:

  1. It's my opinion that one should not merge code contributions of their own into open source projects without agreement from other contributors, which there isn't at present.
  2. None of this change is documented in the readme. Without documenting the caching abilities this adds, no one will know it is there to use.

@lcobucci
Copy link
Collaborator Author

lcobucci commented Jun 21, 2021

@designermonkey thanks for sharing your thoughts.

  1. It's my opinion that one should not merge code contributions of their own into open source projects without agreement from other contributors, which there isn't at present.

Indeed, not have a review isn't ideal. However, we've got extensive tests and benchmarks that give enough information not to block this PR.
Nikita is extremely busy with many things and my hours to work on this particular project are limited. So, I had to make a call to move things forward.

I do invite you to review the code and share your findings here or on a PR.

  1. None of this change is documented in the readme. Without documenting the caching abilities this adds, no one will know it is there to use.

I've indeed overlooked the documentation and will correct my mistake ASAP. In the meantime, feel free to send a PR - maybe you're faster than me.

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