search

Payum / PayumBundle

Payum offers everything you need to work with payments. From simplest use cases to very advanced ones.
https://payum.gitbook.io/payum
MIT License
569 stars 142 forks source link

Use Symfony service to register templates instead of TwigUtil #518

Open uwej711 opened 3 years ago

uwej711 commented 3 years ago

TwigUtil uses a static SplObjectStorage when registering templates. This leads to memory leaks when you have a lot of funtional tests. Registering templates through a service avoids that memory leak.

uwej711 commented 3 years ago

In our case (combined with other issues) this reduces the memory foot print of our test suite from over 10 GB to less than 5 GB.

davidmpaz commented 3 years ago

Hi Uwe,

if you don't mind i will try to understand everything while reviewing this PR. Would it be possible to include some profiling data of the improvements to get more insights?

Also, could you please take a look into this https://github.com/Payum/Payum/issues/859. I am wondering if we can address this problem direct in Payum core itself. Do you think it would be possible to do a port this to there, totally or partially?

I will appreciate any kind of feedback.

thanks in advance for taking care, Best regards, David

uwej711 commented 3 years ago

Hi David, our situation is exactly the same as in the issue you mentioned. The problem is that the SplObjectStorage keeps that twig and loader in memory forever which keeps php from freeing the memory of these objects. Depending on your application that will keep a lot more objects in memory, e.g. each Symfony kernel that is created in each of your functional tests.

I thought about changing the code in core as well, but there it is either getting rid of the SplObjectStorage or adding a "clear" method as described in the issue. Both solutions would have forced us to use a fork of core for some time so I came up with this solution which works nice for applications using Symfony and the bundle. This solution basically frees the memory when the Symfony kernel is shutdown.

My idea for core would be to simply check for an existing fileloader or attach a new one with chain loader without keeping the SplObjectStorage, another naive solution would be to always add a new fileloader to the chainloader, but that seems not ideal.

Using an approach similar to the one in this PR would probably require quite a few changes in core as it would require to get completely getting rid of the static methods and class members and make some parts of the core more dependency injection friendly which would break current usage.

davidmpaz commented 3 years ago

Hi Uwe,

thanks for clarifying. Do you feel adventurous ;-) with this?

Using an approach similar to the one in this PR would probably require quite a few changes in core as it would require to get completely getting rid of the static methods and class members

If you can afford some time for making a pull request there in core would be great since you already know the issue.

Otherwise i would do my shot by doing what i always do in cases like this one; make an interface and provide current implementation (TwigUtil) as default implementation allowing to clients to re-implement the whole thing similar to what you did, actually i would be taking as reference your implementation :-).

I would also remove any statics from the so called default implementation and injecting it somehow wherever is needed, which, by the way it is only used in \Payum\Core\CoreGatewayFactory it seems it should not be difficult to refactor. Maybe it is only me being ignorant.

One question that remains for me is the root cause for this to exist. @makasim maybe could you provide some hints on why \Payum\Core\Bridge\Twig\TwigUtil was needed? Because of your comment in the commit:

do not register loader second time

and from the very same code i can infer it was intended to implement some kind of singleton or cached storage but can not be sure completely. Also, is there any danger in registering loaders 2 times? As i see it, Twig will use the first one finding the templates.

uwej711 commented 3 years ago

Hi David,

I can try to work on the issue in core, but it will take some time as I'm quite busy at the moment. So if you come up with something earlier just let me know.

pierredup commented 3 years ago

Instead of creating a new service to manipulate the twig loader, I would suggest to prepend the container config and add the twig paths as part of the container config. That way, it would benefit from all the container optimizations and compiled cache

pierredup commented 3 years ago

@uwej711 Do you still want to finish this PR?

davidmpaz commented 3 years ago

Instead of creating a new service to manipulate the twig loader, I would suggest to prepend the container config and add the twig paths as part of the container config. That way, it would benefit from all the container optimizations and compiled cache

Hi @pierredup thanks for contributing your ideas. One question from my side: Will your solution solve the problem only for the symfony bundle?

Thanks in advance, David

pierredup commented 3 years ago

@davidmpaz Yes, it will solve it only for the bundle. In Payum core, we might need to look at another solution (maybe even getting rid of SplObjectStorage altogether)

uwej711 commented 3 years ago

Sorry for being unresponsive ... quite busy atm. The idea was to fix this in the library as mentioned above (https://github.com/Payum/PayumBundle/pull/518#issuecomment-714735953).

I will try to tackle that in the next few days. Then this PR should be obsolete.

pierredup commented 3 years ago

Prepending the container configuration with the twig paths would still have a benefit when using the Symfony bundle. So even with a fix in core, there can still be some improvement in the bundle (as it's not just about the memory usage then anymore, but about an optimized container cache when using the bundle)

davidmpaz commented 3 years ago

Hi,

Sorry for being unresponsive ... quite busy atm. The idea was to fix this in the library as mentioned above (#518 (comment)). I will try to tackle that in the next few days. Then this PR should be obsolete.

thank you Uwe, no problems, we are all busy ;)

Prepending the container configuration with the twig paths would still have a benefit when using the Symfony bundle. So even with a fix in core, there can still be some improvement in the bundle (as it's not just about the memory usage then anymore, but about an optimized container cache when using the bundle)

I lack of experience here. Could we bring this into consideration for the other Team members? Maybe in a separate pull request?

thank you guys for your time, David