snippet performance improvement

[wardi]

Fixes #6146

Proposed fixes:

reduce separate rendering steps for {% snippet %} by converting to:

{% with %}
    {% import ... as snippet %}
    {{ snippet(...) }}
{% endwith %}

and a generated template with

{% macro snippet(...) %}
    {% include ... %}
{% endmacro %}

for fewer extra stack frames and better performance.

Note

does not improve performance of h.snippet and other helpers that call it, like h.user_image, h.follow_button, h.csrf_input

---
config:
    xyChart:
        height: 300
---
xychart-beta horizontal
    title "/testing/primer performance (23 snippets)"
    x-axis ["old", "new"]
    y-axis "Speed (responses/s)" 0 --> 11
    bar [5.18, 6.25]

Features:

• includes tests covering changes
• includes updated documentation
• includes user-visible changes
• includes API changes
• includes bugfix for possible backport

[wardi]

It's a little concerning that ckan/tests/lib/search/test_query.py::TestPackageQuery::test_name_multiple_results is a flaky test 🤔

[wardi]

Another benefit is the stack traces and profile outputs of rendering templates are much cleaner. Here's the existing code with a {% snippet %} that inserts 8 stack frames and makes profiling templates almost impossible because you can't follow from the parent template to the rendered snippet (all profile data from snippets is mixed together)

After this change there are 0 additional stack frames and profiling is easy:

[smotornyuk]

Suggestion for simpler migration/detecting cached templates.

In addition to currently supported forms of snippet

• {% snippet "name.html" %}
• {% snippet "name.html", var=value %}

let's add another one: {% snippet "name.html" cached %}. This form will be completely identical to {% snippet "name.html" %} and does not change behavior.

In addition, let's add config option or environment variable that reports any usage of simplest form {% snippet "name.html" %}.

In this way a developer can enable this switch and see all the templates, that are cached and may be outdated, just as promotedt.html in your PR. After analyzing the snippet, the developer will add cached(if it's safe to cache) or _anything=none(if the template relies on a variable) to the snippet tag.

Because of the cached keyword, the developer can easily filter out processed snippets. As this keyword doesn't change anything, it can be kept in the future if you don't need backward compatibility or are too lazy to remove this keyword. Or it can be removed via regexp when migration is finished and your extension will become compatible with older CKAN versions, while you are sure, it won't have cache problems with newer CKAN versions

[wardi]

@smotornyuk @amercader I've removed caching for snippets without parameters. It seems jinja2 caching ignores everything, including the language setting, making it useless on many sites and not worth the breaking change for the remaining ones.

[wardi]

Just found a problem with variables in the parent templates leaking into the snippets. Working on a different approach using {% import %} and {% macro %} instead.

[amercader]

I assume this is deliberately left out from globals?

[wardi]

I didn't run into any failing tests, but some extensions might depend on it so I'll move it too

[amercader]

I must confess that I don't fully grasp this code. Would this be a fair summary?
Here we parse the {% snippet 'path/to/snippet.html', arg1=test %} string, and turn it into a node tree equivalent to:

{% with %}
    {% import ... as snippet %}
    {{ snippet(...) }}
{% endwith %}

And then this will be picked up in the CkanFileSystemLoader.parse() method above which will turn it into :

{% macro snippet(...) %}
    {% include ... %}
{% endmacro %}

Which is what will be finally rendered by Jinja. Or is this the other way around?
Sorry I find this a bit confusing

[wardi]

That's right. The snippet with/import node tree references a specially named wrapper template original_name§arg1,arg2.html that gets generated by CkanFileSystemLoader.get_source() containing macro/include tags.

The imported wrapper template is necessary to create a separate namespace and convert arg1, arg2… into local variables for the snippet template.

[wardi]

The old code has 7 stack frames between snippet tag and the actual snippet html:

   File "/srv/app/src_extensions/ckan/ckan/templates/development/primer.html", line 21, in block 'secondary_content'
     {% snippet 'development/snippets/nav.html', heading='Navigation' %}
   File "/srv/app/src_extensions/ckan/ckan/lib/jinja_extensions.py", line 277, in _call
     return base.render_snippet(*args, **kwargs)
   File "/srv/app/src_extensions/ckan/ckan/lib/base.py", line 69, in render_snippet
     output = render(template_name, extra_vars=kw)
   File "/srv/app/src_extensions/ckan/ckan/lib/base.py", line 104, in render
     return flask_render_template(template_name, **extra_vars)
   File "/usr/lib/python3.10/site-packages/flask/templating.py", line 150, in render_template
     return _render(app, template, context)
   File "/usr/lib/python3.10/site-packages/flask/templating.py", line 131, in _render
     rv = template.render(context)
   File "/usr/lib/python3.10/site-packages/jinja2/environment.py", line 1304, in render
     self.environment.handle_exception()
   File "/usr/lib/python3.10/site-packages/jinja2/environment.py", line 939, in handle_exception
     raise rewrite_traceback_stack(source=source)
   File "/srv/app/src_extensions/ckan/ckan/templates/development/snippets/nav.html", line 1, in top-level template code

Now there are only 2 stack frames (including the generated intermediate development/snippets/nav§heading.html template)

   File "/srv/app/src_extensions/ckan/ckan/templates/development/primer.html", line 21, in block 'secondary_content'
     {% snippet 'development/snippets/nav.html', heading='Navigation' %}
   File "/usr/lib/python3.10/site-packages/jinja2/runtime.py", line 782, in _invoke
     rv = self._func(*arguments)
   File "development/snippets/nav§heading.html", line 1, in template
   File "/srv/app/src_extensions/ckan/ckan/templates/development/snippets/nav.html", line 1, in top-level template code

[amercader]

This looks good to me @smotornyuk, but feel free to have another look

Once the last changes are done I'll merge it