Skip to content

perf: update template renderer paths to use package qualifiers #18277

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

Merged
merged 3 commits into from
Jul 18, 2025

Conversation

brettlangdon
Copy link
Contributor

@brettlangdon brettlangdon commented Jun 16, 2025

Avoid unnecessary template lookup failures by specifying full package paths for all template renderers. Previously, for endpoints like /simple/<package>, the template resolver would first attempt to find 'warehouse.api:api/simple/detail.html' which would always fail, then fall back to 'templates/api/simple/detail.html'. By using explicit package qualifiers like 'warehouse:templates/api/simple/detail.html', we skip the initial failed lookup and exception handling.

This provides a small performance optimization by eliminating known failed disk lookups, as well as avoiding exception traceback generation when ddtrace is enabled (for attaching the traceback to the span metadata).

Testing locally, the template load time goes from ~830us to ~80us, and template rendering time goes from ~4ms to ~3.7ms... a small optimization, but an easy win.

Avoid unnecessary template lookup failures by specifying full package
paths for all template renderers. Previously, for endpoints like
/simple/<package>, the template resolver would first attempt to find
'warehouse.api:api/simple/detail.html' which would always fail, then
fall back to 'templates/api/simple/detail.html'. By using explicit
package qualifiers like 'warehouse:templates/api/simple/detail.html',
we skip the initial failed lookup and exception handling.

This provides a small performance optimization by eliminating known
failed disk lookups, as well as avoiding exception traceback
generation when ddtrace is enabled.
@brettlangdon brettlangdon requested a review from a team as a code owner June 16, 2025 19:36
@brettlangdon
Copy link
Contributor Author

What should we do about:

# We'll store all of our templates in one location, warehouse/templates
# so we'll go ahead and add that to the Jinja2 search path.
config.add_jinja2_search_path("warehouse:templates", name=".html")
config.add_jinja2_search_path("warehouse:templates", name=".txt")
config.add_jinja2_search_path("warehouse:templates", name=".xml")

Is this proposed change even desired for the minimal gain vs having the default search path?

@brettlangdon
Copy link
Contributor Author

brettlangdon commented Jun 16, 2025

An alternative here is to move all the templates into their respective module paths, e.g. warehouse/templates/api/simple/detail.html -> warehouse/api/templates/simple/detail.html and then update renderer="templates/simple/detail.html" (since warehouse.api:will get prepended to all template lookups by default which don't have:` in the name)

Copy link
Member

@miketheman miketheman left a comment

Choose a reason for hiding this comment

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

I generally like this, as it makes it more explicit and clear which template is where.

Sadly, the functional tests (webtest-style tests) don't cover every @view_config instance to validate that the changes are safe, and we'd only find out at runtime if the template is truly missing.

Can you conceive of a way to validate that each of these template paths exists in a lint/test phase, or another way to verify that these string changes don't break at runtime?

@miketheman
Copy link
Member

#18366 likely solves the concern, so once that lands, this PR should be checked against that for accuracy.

@miketheman
Copy link
Member

@brettlangdon looks like the checker I wrote doesn't yet account for the updated paths - would you want to take a stab at adding that?

@miketheman
Copy link
Member

@brettlangdon looks like the checker I wrote doesn't yet account for the updated paths - would you want to take a stab at adding that?

I did it :P #18388

Copy link
Member

@miketheman miketheman left a comment

Choose a reason for hiding this comment

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

With the checker in place, I now have more confidence in this change. Thanks @brettlangdon !

@miketheman miketheman merged commit 3bc1006 into pypi:main Jul 18, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants