[Django] #20806: Cached template loader doesn't cache find_template
Django
---------------------------------+---------------------
Reporter: gwahl@… | Owner: gwahl@…
Type: Uncategorized | Status: new
Component: Template system | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+---------------------
`django.template.loaders.cached` memoizes the result of `load_template`,
but not `find_template`. I have an application that calls
`select_template([a long list])` many times, and `find_template` tries to
find the same templates 100s of times. Adding a cache to `find_template`
resulted in a huge speedup for my application.
It's pretty easy to implement this and results in a nice speedup. I'll
attach a patch shortly.
--
Ticket URL: <https://code.djangoproject.com/ticket/20806>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: gwahl@… | Owner: gwahl@…
Type: Uncategorized | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by gwahl@…):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
Comment:
Patch/pull request: https://github.com/django/django/pull/1400
--
Ticket URL: <https://code.djangoproject.com/ticket/20806#comment:1>
Django
Reporter: gwahl@… | Owner: gwahl@…
Type: New feature | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* type: Uncategorized => New feature
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Thanks for the suggestion and pull request. IMO a change like this does
need a test; not so much to verify the performance gain, as simply to give
coverage of the added lines/branches for the "found in cache" case.
Without that coverage, a change could break that code path and the tests
would not reveal the breakage.
--
Ticket URL: <https://code.djangoproject.com/ticket/20806#comment:2>
Django
Reporter: gwahl@… | Owner: gwahl@…
Type: New feature | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by gwahl@…):
The `find_template` method actually already has 100% coverage, including
the code paths for 'found in cache' and 'not found in cache'. I'll attach
the coverage report.
I did notice that there's no test for adding `template_dirs` to the cache
key (this was added in #13573, but there was no test). Would it be
appropriate to add the test for that here?
--
Ticket URL: <https://code.djangoproject.com/ticket/20806#comment:3>
Django
Reporter: gwahl@… | Owner: gwahl@…
Type: New feature | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by carljm):
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
If we've already got coverage of both those code paths, then I think I'm
ok with this performance enhancement going in without an additional test.
I don't see it as real useful to add a test actually asserting that the
second `find_template` doesn't go back to the filesystem.
Thanks for noticing the lack of coverage for `template_dirs` in cache key!
There actually was a test for that added in #13573 (see 8a6cb3d969), but
that test was never run (`CachedLoaderTests` was not imported in
`tests.py`). I've fixed that just now in 8f3aefdec33.
Marking this RFC as it looks good to me and tests pass, but I'd like
someone else to confirm that it's ok to go in without additional tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/20806#comment:4>
Django
Reporter: gwahl@… | Owner: gwahl@…
Type: New feature | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by jezdez):
Yep, looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/20806#comment:5>
Django
Reporter: gwahl@… | Owner: gwahl@…
Type: New feature | Status: closed
Component: Template system | Version: master
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"5154c9f92caa38bd5893320ed72fbc7305233822"]:
{{{
#!CommitTicketReference repository=""
revision="5154c9f92caa38bd5893320ed72fbc7305233822"
Fixed #20806 -- Cached loader caches find_template
The cached template loader should cache find_template in addition to
load_template.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20806#comment:6>
Django
Reporter: gwahl@… | Owner: gwahl@…
Type: New feature | Status: closed
Component: Template system | Version: master
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz <claude@…>):
In [changeset:"3ac823fc5b88db7808f4d1cbca122713c6c9e03a"]:
{{{
#!CommitTicketReference repository=""
revision="3ac823fc5b88db7808f4d1cbca122713c6c9e03a"
Fixed #21460 -- Reenabled proper template precedence in find_template
Refs #20806. Thanks Unai Zalakain for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20806#comment:7>