Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20806 closed New feature (fixed)

Cached template loader doesn't cache find_template

Reported by: gwahl@… Owned by: gwahl@…
Component: Template system Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


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.

Attachments (1),cover (3.2 KB) - added by gwahl@… 3 years ago.
coverage report for cached template loader

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by gwahl@…

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 3 years ago by Carl Meyer

Needs tests: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

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.

Changed 3 years ago by gwahl@…

Attachment:,cover added

coverage report for cached template loader

comment:3 Changed 3 years ago 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?

comment:4 Changed 3 years ago by Carl Meyer

Needs tests: unset
Triage Stage: AcceptedReady for checkin

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 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.

comment:5 Changed 3 years ago by Jannis Leidel

Yep, looks good to me.

comment:6 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 5154c9f92caa38bd5893320ed72fbc7305233822:

Fixed #20806 -- Cached loader caches find_template

The cached template loader should cache find_template in addition to

comment:7 Changed 3 years ago by Claude Paroz <claude@…>

In 3ac823fc5b88db7808f4d1cbca122713c6c9e03a:

Fixed #21460 -- Reenabled proper template precedence in find_template

Refs #20806. Thanks Unai Zalakain for the review.

Note: See TracTickets for help on using tickets.
Back to Top