Code

Opened 6 years ago

Closed 5 years ago

#9874 closed (duplicate)

Added template cache in template loader

Reported by: novoya Owned by: novoya
Component: Template system Version: master
Severity: Keywords: template cache
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The following view module fails to make good reuse to the template
For each new request, all inherited/included templates will be recreated and compiled

tpl = loader.get_template('mytemplate.html')

def view_func1(request):
     '''
        Any inherited templates from tpl
        will created and recompiled again and again
     '''
     return HttpResponse(tpl.render(context)) 

Attached a very simple patch to solve the problem:
1.Simple template caching in the django.template.loader
2.Minimum change in django.template.loader_tags

Attachments (2)

template_cache.diff (2.0 KB) - added by novoya 6 years ago.
templace cache patch
template-cache.diff (6.9 KB) - added by groks 5 years ago.
Fixes for cache key name, thread safety. Adds config, tests.

Download all attachments as: .zip

Change History (16)

Changed 6 years ago by novoya

templace cache patch

comment:1 Changed 6 years ago by novoya

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to novoya
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 6 years ago by novoya

  • milestone set to post-1.0
  • Needs tests set
  • Resolution set to fixed
  • Status changed from assigned to closed
  • Summary changed from Template reuse is worthless(when inheretiance or inclusion involved) because parent templates get re-compiled every time to Added template cache in template loader

comment:3 Changed 6 years ago by ElliottM

I'm confused, when was this fixed?

comment:4 Changed 6 years ago by ubernostrum

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:5 follow-up: Changed 6 years ago by ubernostrum

  • Triage Stage changed from Unreviewed to Design decision needed

It wasn't fixed. Somebody needs to learn how to read the documentation on the ticket process to find out what "fixed" means (hint: not "some random person uploaded a patch").

comment:6 follow-up: Changed 6 years ago by mtredinnick

This is almost certainly going to cause rednering bugs. The Python objects representating templates are not just the compiled form, they also (intentionally) contain rendering state. It also assumes that the same name will load the same template in every thread, which isn't true (a dynamic template loader, for example). So there are larger considerations that don't seem to have been addressed here.

Right now, this is probably the wrong approach to solving the perceived problem, but want to think about it some more.

comment:7 Changed 6 years ago by lukeplant

It also is a hit in terms of memory usage -- every single template used in the system is going to get stored in memory, if I read the patch correctly. This could be a much worse penalty than any benefit it gives. I read somewhere that Adrian and co had originally intended templates to be stored in some compiled form, but they realised later that it simply wasn't worth the overhead.

comment:8 in reply to: ↑ 5 Changed 6 years ago by novoya

Replying to ubernostrum:

It wasn't fixed. Somebody needs to learn how to read the documentation on the ticket process to find out what "fixed" means (hint: not "some random person uploaded a patch").

Yes. Sorry guys. My mistake. I isn't fixed
Anyway, I didn't tested too much, but from my tests, my SIMPLE fix boosts performance considerably (for each refresh- 0 template reloads + compiles instead of 5 or more in a typical web app)
About memory usage concerns, it is very easy to limit the size of the cache in code.

comment:9 in reply to: ↑ 6 Changed 6 years ago by novoya

Replying to mtredinnick:

This is almost certainly going to cause rednering bugs. The Python objects representating templates are not just the compiled form, they also (intentionally) contain rendering state. It also assumes that the same name will load the same template in every thread, which isn't true (a dynamic template loader, for example). So there are larger considerations that don't seem to have been addressed here.

Right now, this is probably the wrong approach to solving the perceived problem, but want to think about it some more.

How can template contain rendering state, if each time a template needed, it gets reloaded and recompiled ??

comment:10 Changed 5 years ago by simon29

Some benchmarking might be useful here. I could imagine the benefit being negligent on simple templates; but it could give a big plus for high traffic templates and/or large, complex templates (eg with lots of includes, multiple levels of inheritance, loading lots of tag libraries). Maybe it could be a setting, eg TEMPLATE_CACHE = 100, set zero by default. I have noticed before that after the database has been optimised and memcache used wherever possible, a unnecessarily high number of cycles are chewed up on template rendering.

comment:11 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

Changed 5 years ago by groks

Fixes for cache key name, thread safety. Adds config, tests.

comment:12 Changed 5 years ago by groks

  • Needs tests unset

Y'all, here's a new patch which fixes a couple of the concerns raised above:

Add TEMPLATE_CACHE config setting (default off), update the docs.
Don't cache templates when we're in TEMPLATE_DEBUG mode.
Use auxiliary template load dirs as part of cache key name.
Add some locking around the template cache.
Add some tests.

Not only is template caching faster, but it opens the door for further tuning. For example, we could walk the compiled template and coalesce adjacent TextNode's.

What else needs to be done to get this accepted?

comment:13 Changed 5 years ago by ramiro

See also #9154, it contains a patch with a similar implementation and it seems to take in account problems found with the {{ block.super}} variable.

comment:14 Changed 5 years ago by mrts

  • Resolution set to duplicate
  • Status changed from reopened to closed

Duplicate of #9154.

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.