Skip to content

[DependencyInjection] [WIP] add a #[Memoize] method attribute #47099

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 1 commit into from

Conversation

Jean-Beru
Copy link
Contributor

@Jean-Beru Jean-Beru commented Jul 28, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

Memoization is a common technique to avoid heavy computation by caching previous returned value.

This is a proposal to add #[Memoize] method attribute to Symfony in order to return a cached response using Cache
component. Actually more a POC than PR.

Proposed Usage:

#[Memoizable]
class MyService
{
    public function __construct(private readonly HttpClientInterface $client)
    {
    }

    #[Memoize(pool: 'cache.redis_pool', ttl: 5)]
    public function getSomeEndpoint(string $name): string
    {
        return $this->client->request('GET', '/api/some/endpoint')->getContent();
    }

    #[Memoize(pool: 'cache.pdo_pool', keyGenerator: CustomKeyGenerator::class, ttl: 3600)]
    public function getThatChangesRarely(string $name): string
    {
        return $this->client->request('GET', '/api/that/changes/rarely')->getContent();
    }
}

Questions:

Before going further (refactoring, adding tests, etc.), it could be a good thing to get some comments because
each solution could take a lot of time to implement :

  • How the service is created
    • By replacing the current service by a factory which will create the "proxy" from extended class used in this
      PR
      . Final services won't be supported used in this PR due to ProxyManager.
    • By decorating the service (see last point). Interfaces have to be kept, all functions implemented and magic
      methods implemented to retrieve/call public properties/methods.
  • How to generate and dump the file
    • By using the AccessInterceptorValueHolderFactory if ProxyManager bridge is installed used in this PR.
    • By using a Generator like AccessInterceptorValueHolderGenerator to generate the class and using a custom
      PhpDumper to save. Something similar as it's done for lazy services.
    • By generating manually, it can be a hard task to handle all specific cases.

Is it new?

A bundle already exists about memoization: https://github.com/RikudouSage/SymfonyMemoizeBundle but this PR differs:

  • Cache can be configured (which cache, TTL, key generation).
  • Generated classes use inheritance instead of decoration (for now, like lazy proxy does).

@carsonbot carsonbot added Status: Needs Review DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Jul 28, 2022
@carsonbot carsonbot added this to the 6.2 milestone Jul 28, 2022
@carsonbot carsonbot changed the title [DependencyInjection] [RFC] [WIP] add a #[Memoize] method attribute [DependencyInjection] [WIP] add a #[Memoize] method attribute Jul 28, 2022
Copy link
Contributor

@michaljusiega michaljusiega left a comment

Choose a reason for hiding this comment

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

I like this.
I left some comments before going sleep :)

*/
public function __invoke(string $className, string $method, array $arguments): string
{
return hash('sha256', $className.$method.serialize($arguments));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remember. You don't know what is inside in $arguments variable. May be anything, for example a closure instance that's impossible to serialize here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree. The KeyGenerator is configurable to allow user to handle this kind of specific cases.

namespace Symfony\Component\DependencyInjection\Attribute;

#[\Attribute(\Attribute::TARGET_CLASS)]
class Memoizable
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we can skip this class as attribute, useless double check in compiler pass :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used it to make a first filter instead of retrieving all services' method attributes.

By the way, this attribute could contain default configuration like TTL or cache pool.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, good idea. ;)

@Warxcell
Copy link
Contributor

What about just using HttpCacheKernel?

@Jean-Beru
Copy link
Contributor Author

What about just using HttpCacheKernel?

You're right, HttpCacheKernel is better if you want to cache a Response.
However, this PR will allow to cache any method results (database requests, heavy computations, etc.).

@nicolas-grekas
Copy link
Member

Thanks for the proposal.
On my side, I think this is a lot of code infrastructure, challenges (compile/runtime code generation) and third party dependencies for a use case that is solved in just a few lines of code when using CacheInterface.
For these reasons, I don't think this should be in Symfony core.
Note that if you decide to maintain this feature in a dedicated package, I would recommend trying to use CacheInterface instead of PSR-6 as this will give you eg stampede protection out of the box.

@nicolas-grekas
Copy link
Member

Another note: once #47236 is merged, it should be possible to replace proxy-manager by code generation based on reflection + ProxyHelper::exportSignature(). I'd be happy to know how this goes if you want to give it a try.

@Jean-Beru
Copy link
Contributor Author

Thanks for the review! Indeed, this PR will bring a lot of code to maintain.
I think I will give a try to #47236 in a dedicated library when my parental leave will end (I need some sleep before 😅)

@fabpot
Copy link
Member

fabpot commented Aug 23, 2022

Closing then. Thank you @Jean-Beru and congrats! 👶

@fabpot fabpot closed this Aug 23, 2022
@Jean-Beru Jean-Beru deleted the di/memoize branch June 5, 2024 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Needs Work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants