Django

Code

Ticket #9154 (closed: duplicate)

Opened 1 year ago

Last modified 11 months ago

Cache compiled templates

Reported by: msaelices Assigned to:
Milestone: Component: Template system
Version: SVN Keywords: templates, cache
Cc: lemuelf@sectioneleven.org, Reflejo@gmail.com, german.mb@gmail.com Triage Stage: Design decision needed
Has patch: 1 Needs documentation: 1
Needs tests: 1 Patch needs improvement: 0

Description

I've seen compiled templates are not cached in loader class. You can improve performance in environments like Apache with only caching compiled templates by name. You can rely that a same string content in a template will became a same compiled template (I forgot here the threading problem, that is another issue).

I will attach a patch that can improve rendering in hundred times (it depends of cache hit). In one production site, I gain from 15rps to 31rps with apache bench tool (that timing include SQL sentences, logic, etc.), so the template gain is much better.

Attachments

templates_optimizations_r9076.diff (4.3 kB) - added by msaelices on 09/19/08 22:01:04.
Templates system optimization
templates_optimizations_r9076.2.diff (4.6 kB) - added by msaelices on 09/19/08 23:41:16.
New patch that fixes a rare inheritance case.
templatebench.py (1.6 kB) - added by msaelices on 09/20/08 00:19:05.
A trivial bench
templates_optimizations_r9109.diff (4.5 kB) - added by msaelices on 10/01/08 16:02:33.
New patch with SmileyChris? comments fixed
templates_optimizations_r9235.diff (3.6 kB) - added by msaelices on 10/19/08 18:46:31.
New patch without ticket #8110 patch inside (it was fixed)
templates_optimizations_r9285.diff (4.5 kB) - added by msaelices on 10/27/08 20:17:05.
New patch that fixes {{ block.super }} issue (updated to r9285)
#9154 - templates_optimizations.diff (5.5 kB) - added by Kronuz on 05/05/09 00:16:34.
New patch that hopefully fixes all {{ block.super }} issues
#9154 - templates_optimizations.2.diff (5.4 kB) - added by Kronuz on 05/05/09 04:01:20.
same as the previous but with deepcopy removed

Change History

09/19/08 22:01:04 changed by msaelices

  • attachment templates_optimizations_r9076.diff added.

Templates system optimization

09/19/08 22:03:14 changed by msaelices

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

Noto: patch includes another mini-patch made in #8110 (only in django/contrib/admin/templatetags/log.py file). Without this patch, for example, admin site fails second time I visite it.

(follow-up: ↓ 3 ) 09/19/08 22:06:52 changed by msaelices

Sorry for my little description. One point of optimization is this: previous patch, if a template has inheritance from another, parent compiled template is not cached. This is the reason in profiling for having high load in Lexer all the time, mentioned in this thread, thats compare Jinja templates to Django ones.

(in reply to: ↑ 2 ) 09/19/08 22:08:18 changed by msaelices

Replying to msaelices:

Sorry for my little description. One point of optimization is this: previous patch, if a template has inheritance from another, parent compiled template is not cached. This is the reason in profiling for having high load in Lexer all the time, mentioned in this thread, thats compare Jinja templates to Django ones.

s/Lexer/parsing :-P

09/19/08 22:31:21 changed by msaelices

I've found the patch doesn't works in other inner links in my project. I will search for problem.

09/19/08 22:43:35 changed by msaelices

Problem is in block inheritance.

09/19/08 23:40:31 changed by msaelices

I've found and fixed the error. It's caused when you have this kind of base template: 

# base.html
<html>
<body>
{% block wrapper %}
  Lorem ipsum...
  {% block content %}
  {% endblock %}
{% endblock %}
</body>
</html>

# first.html
{% extends "base.html" %}
{% block wrapper %}
  Foo content.
{% endblock %}

# second.html
{% extends "base.html" %}
{% block content %}
  Bar content.
{% endblock %}

The problem exists because child rendering replaces compiled_parent.nodelist enterely. The first link, and first.html rendering, put on cache a base.html template with only one block node name wrapper. In second link, when second.html gets rendered, block content doesn't exists in cached compiled parent, and fails silently without changing nothing.

I fixed that making a deepcopy of compiled_parent :-)

09/19/08 23:41:16 changed by msaelices

  • attachment templates_optimizations_r9076.2.diff added.

New patch that fixes a rare inheritance case.

09/20/08 00:19:05 changed by msaelices

  • attachment templatebench.py added.

A trivial bench

09/20/08 00:22:25 changed by msaelices

I've attached a simple benchmarking. This benchmarking is not very realistic, because I override default loader for not load from file system.

The results on my machine, before patching Django: 

$ ./manage shell
>>> import templatebench
>>> templatebench.test()
Bench duration: 17.299939

After optimization: 

$ ./manage shell
>>> import templatebench
>>> templatebench.test()
Bench duration: 9.878777

I think if you add IO things, I think improvement is very high.

09/22/08 16:38:20 changed by msaelices

Another possibility is use hashed content as cache key, but I think is not necessary. I have to test a multiple sites scenario (with one apache with several virtual hosts), but I'm almost sure that it will not fail.

(follow-up: ↓ 12 ) 09/24/08 19:56:20 changed by SmileyChris

Related ticket: #6262

Good things:

  • You're getting the cache templates discussion going again :)
  • Nice catch on the template inheritance problem

Things that need fixing:

  • You're not setting user_id under all circumstances in django/contrib/admin/templatetags/log.py
  • You can't remove get_parent() because the parent template could be based on a context variable. It needs to be figured out at render time.

Other considerations:

  • Probably needs a setting to turn off caching if you don't want it. In fact, it probably should be off by default to keep full backwards compatibility (introduces the need to reset the server after changing templates).

I was thinking that if people were really worried about out-of-date cached templates then there could be some a cache which compares file size or date of the file. This would obviously be a slight bit slower and is out of scope for an initial cut - just writing down my thoughts :)

(follow-up: ↓ 13 ) 09/24/08 19:59:27 changed by SmileyChris

Also:

  • The new functionality of a dirs argument on get_template doesn't belong in this ticket
  • And I'm guessing from_child argument slipped in there by mistake?

09/26/08 22:54:23 changed by mtredinnick

It feels like this sort of thing is better done as a template loader that accepts any other template loader and caches the result (basically a wrapper around other loaders). That saves having to make the same kind of internal modifications in every case. I'd prefer that type of approach, since it's really just caching an compiled template object, however that happened to have been loaded.

In addition to the things Chris mentioned, if you think there's some inheritance problem that is unrelated to this caching feature addition, please make that a separate ticket. If it's only relevant with this enhancement, then one patch is appropriate, rather than multiple patches. But we need to keep the bug fixes and the feature additions separate.

One question I always have with this sort of thing is what the behavioural changes are going to be (since they need to be documented). If a template contains a tag that returns a random number each time the template is rendered, how will that be changed with this sort of thing? Will the tag be re-evaluated each time? That's going to be necessary to mention in the documentation changes that haven't been written yet.

Somebody (you and SmileyChris?, basically) have to decide which ticket out of this one and #6262 to close as a dupe (it should probably be this one). Having two tickets addressing the same thing doesn't make things any easier for us.

(in reply to: ↑ 9 ) 10/01/08 16:01:03 changed by msaelices

Replying to SmileyChris:

Related ticket: #6262 Good things: * You're getting the cache templates discussion going again :) * Nice catch on the template inheritance problem Things that need fixing: * You're not setting user_id under all circumstances in django/contrib/admin/templatetags/log.py

Sorry, I take this from this patch. I've fixed that.

* You can't remove get_parent() because the parent template could be based on a context variable. It needs to be figured out at render time.

You're right. I've fixed that also.

Other considerations: * Probably needs a setting to turn off caching if you don't want it. In fact, it probably should be off by default to keep full backwards compatibility (introduces the need to reset the server after changing templates).

I think this change is not a feature change. It's only a optimization. Of course, I've changed my patch to use settings.TEMPLATE_DEBUG for turning on/off this caching.

I was thinking that if people were really worried about out-of-date cached templates then there could be some a cache which compares file size or date of the file. This would obviously be a slight bit slower and is out of scope for an initial cut - just writing down my thoughts :)

I think settings.TEMPLATE_DEBUG is enough explicit for developers that wants automatic template reloads.

10/01/08 16:02:33 changed by msaelices

  • attachment templates_optimizations_r9109.diff added.

New patch with SmileyChris? comments fixed

(in reply to: ↑ 10 ) 10/01/08 16:04:28 changed by msaelices

Replying to SmileyChris:

Also: * The new functionality of a dirs argument on get_template doesn't belong in this ticket

Maybe, but I needed that functionality in my patch. I was forced to add dirs argument on get_template to be able to cache templates.

* And I'm guessing from_child argument slipped in there by mistake?

Of course, sorry again. I've fixed that.

10/01/08 16:18:43 changed by msaelices

@mtredinnick, @SmileyChris?, This is my honest opinion: I think that optimizations in #6262 are overdesign. I think using Django cache system is too hard only for caching compiled templates because:

  • It's simpler use a simple global dictionary, like urlresolvers does (done exactly in [5516]). Common sense said me that a simple solution win over complex one.
  • It's faster that all kinds of cache. memcached can be very fast compared with a complex SQL in a remote database, locmem can be fast also, but you will share this with a bunch of cache keys. Database and filesystem cache can be hundreds of times slower. Also it seems that in #6262 even with memcached there is no performance improvements.
  • Cache system needs templates can be pickable, why you have to add a requirement not needed at all?

10/01/08 19:17:45 changed by SmileyChris

Good job on the fixes, msaelices. I'll do another review some time soon. Do you visit #django at all? If so, a chat on there might flesh out some of the discussions more quickly (or google chat [me]@gmail.com)

10/02/08 20:32:27 changed by mtredinnick

  • stage changed from Unreviewed to Design decision needed.

It simply must be possible for this new behaviour ot be disabled (in fact, it should be disabled by default). You're proposing quite a large change in behaviour here: whenever a developer changes a template to fix a typo or something, the web server must be restarted (in effect; whatever is needed to cause the Django process to restart). That is quite a change in operational practices for a lot of places where designers can currently update templates and have them rolled out without a restart being required.

I'm also not amazingly happy with effectively adding a new cache here that tries to hold things in memory. Why not using Django's existing caching framework and, if you're going to do that, why not just supply a caching wrapperaround all existing loaders? By storing stuff in memory you are increasing Django's in-memory footprint (there could be hundreds or thousands of templates involved) and there's no way to do resource management -- as opposed to using a proper cache which manages the amount of memory it will use. The amount of memory used by the urlresolvers is tiny compared to templates and we cache only as last resource because the overhead of the parsing was becoming unacceptably slow when it was done on every request. Template parsing is proportionally much faster (since you only have to parse the the template you requested, not potentially every template in the system as is the case for URL reversing).

This whole ticket feels like something designed for a small-scale situation that simply won't scale well. I'm still -1 on the proposed implementation.

10/19/08 03:52:40 changed by SmileyChris

PS: the admin.templatetags.log fix has been checked in now so that part is redundant (r9233)

10/19/08 18:46:31 changed by msaelices

  • attachment templates_optimizations_r9235.diff added.

New patch without ticket #8110 patch inside (it was fixed)

10/27/08 17:13:02 changed by msaelices

In one of my projects, I've experimented an issue with this patch and {{ block.super }} use. I will found the error.

10/27/08 20:17:05 changed by msaelices

  • attachment templates_optimizations_r9285.diff added.

New patch that fixes {{ block.super }} issue (updated to r9285)

10/30/08 01:04:33 changed by lemuelf

  • cc set to lemuelf@sectioneleven.org.

01/29/09 15:03:31 changed by Reflejo

  • cc changed from lemuelf@sectioneleven.org to lemuelf@sectioneleven.org, Reflejo@gmail.com.
  • owner changed from msaelices to Reflejo.
  • status changed from new to assigned.

01/29/09 15:04:30 changed by Reflejo

  • owner deleted.
  • status changed from assigned to new.

ups.

05/04/09 18:20:44 changed by ramiro

  • needs_docs set to 1.
  • needs_tests set to 1.

#9874 has a patch with a slightly similar implementation that implements a setting to control the cache and that includes tests.

05/05/09 00:15:37 changed by Kronuz

  • cc changed from lemuelf@sectioneleven.org, Reflejo@gmail.com to lemuelf@sectioneleven.org, Reflejo@gmail.com, german.mb@gmail.com.
  • keywords set to templates, cache.

There was a bug in the way {{ block.super }} extended certain parents not defining the block in question.

For parents which did not define a given BlockNode? name defined in the child, the KeyError? exception was raised and the block was added to the parent's ExtendsNode? nodelist. The parent's complete list of BlockNode? was then not fully restored by the last part of the patched render() function, leaving the added BlockNode? in the parent's ExtendsNode? nodelist.

The patch I'm attaching cleans the parent's ExtendsNode?'s nodelist to the state it was before, effectively restoring the full parent's nodelist.

05/05/09 00:16:34 changed by Kronuz

  • attachment #9154 - templates_optimizations.diff added.

New patch that hopefully fixes all {{ block.super }} issues

05/05/09 03:03:23 changed by mrts

  • summary changed from Huge improvements in templates rendering to Cache compiled templates.

As #9874 is duplicate, I closed it.

05/05/09 03:07:21 changed by mrts

  • status changed from new to closed.
  • resolution set to duplicate.

This itself is duplicate of #6262, closing as such.

05/05/09 04:00:19 changed by Kronuz

After the restoration of the parent's original full nodelist by the last part of the render() function, I believe the deepcopy could be unnecesary, not only that, I have encountered some problems in some templates when I use that deepcopy. Further testing is at hand; I'm attaching the patch without the deepcopy.

05/05/09 04:01:20 changed by Kronuz

  • attachment #9154 - templates_optimizations.2.diff added.

same as the previous but with deepcopy removed

05/05/09 10:11:54 changed by mrts

Kronuz, it makes sense to keep the discussion and different patches at the single ticket that's open, #6262. Keeping various tickets open (or alive) for a single issue serves no good.

Add/Change #9154 (Cache compiled templates)




Change Properties
Action