Serious performance issues when including optional templates

[mahagr]

Twig should not throw exceptions when trying to load optional templates such as{% include ['template1', 'template2', 'template3'] %} or {% include 'template' ignore missing %} .

I was looking into an issue in Grav where forms were very, very slow. Grav uses a lot of nested templates with many overrides, especially with forms. As a result Twig tries to load a lot of missing files, throwing exception every time, thousands of times. During the exception, PHP seems to serialize all the objects in the function parameters by using __debugInfo() or if it's not found, falling back to the default serializer. This seems to be repeated for the whole call stack, and in the end the method was called for a single object 10 000 times. Multiply that with all the objects in the Twig context, and I was seeing something like 200 000 __debugInfo() calls in total.

This issue exists from Twig 1 to 3. I did a quick and dirty fix in our Environment override: https://github.com/getgrav/grav/blob/develop/system/src/Grav/Common/Twig/TwigEnvironment.php#L43-L46

After applying my quick and dirty fix, I'm seeing page load times decreasing from ~6s to ~0.3s with some complex pages.

Using exceptions to implement a normal workflow in the application sounds like a bug to me...?

[stof]

This exception was used because exists was not part of the LoaderInterface in 1.x. It has been added through the ExistsLoaderInterface to improve performance in the ChainLoader (and Twig 2.x made it part of LoaderInterface itself). Using exists() in the case of optional includes makes sense to me. This was probably missed because Symfony itself does not contain any such optional include, and so the performance impact was missed. Can you open a PR against the 2.x branch with your change ?

[mahagr]

Thank you for your quick response. I am going to open a PR, but I don't think my approach is the optimal one. There is some local caching going on when loading the template (especially when filesystem checks are turned off) and my quick fix ignores all of that.

I also need to check that we have found most of the cases, as there seem to be still a few exceptions thrown, just not as many as before. Twig 3 simplifies the logic, which may partly fix the issue.

[fabpot]

If the logic is different, I would only optimize Twig 3.

[mahagr]

The logic is the same, Twig 3 just removes some deprecated checks.

I took another look into the code and it looks like that my fix is a good one after all, but it only fixes the exceptions with array of includes. Unfortunately ignore missing has been implemented by catching exceptions and needs to be fixed locally inside the (each) Node and Template classes.

I'm wondering if I should make separate PRs for each. I'm also wondering what would be the best fix for the ignore missing issue; perhaps instead of catching the exception, Template::loadTemplate() could have extra parameter and return null? Or I could create another method loadOptionalTemplate(): ?Template|TemplateWrapper and instead of try/catch check null.

Both ways would be backward compatible, but require additional changes in Node classes.

[stof]

Well, to me, your patch is the right fix for the case of array of includes. The case of ignore missing can be optimized separately from that.

[GromNaN]

Maybe, the case of single template should be optimized by removing the array & foreach when is_array($names) is false.

[mahagr]

@stof I added a pull request to the first issue. I will need to look deeper to figure out the best way to handle ignore missing, too.