Code

Opened 9 months ago

Closed 9 months ago

Last modified 5 months 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

Description

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)

cached.py,cover (3.2 KB) - added by gwahl@… 9 months ago.
coverage report for cached template loader

Download all attachments as: .zip

Change History (8)

comment:1 Changed 9 months ago by gwahl@…

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

comment:2 Changed 9 months ago by carljm

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to New 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 9 months ago by gwahl@…

coverage report for cached template loader

comment:3 Changed 9 months 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 9 months ago by carljm

  • Needs tests unset
  • Triage Stage changed from Accepted to Ready 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 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.

comment:5 Changed 9 months ago by jezdez

Yep, looks good to me.

comment:6 Changed 9 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 5154c9f92caa38bd5893320ed72fbc7305233822:

Fixed #20806 -- Cached loader caches find_template

The cached template loader should cache find_template in addition to
load_template.

comment:7 Changed 5 months ago by Claude Paroz <claude@…>

In 3ac823fc5b88db7808f4d1cbca122713c6c9e03a:

Fixed #21460 -- Reenabled proper template precedence in find_template

Refs #20806. Thanks Unai Zalakain for the review.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.