#9154 closed Uncategorized (duplicate)
Cache compiled templates
| Reported by: | Manuel Saelices | Owned by: | |
|---|---|---|---|
| Component: | Template system | Version: | dev |
| Severity: | Normal | Keywords: | templates, cache |
| Cc: | lemuelf@…, Reflejo@…, german.mb@…, German M. Bravo | Triage Stage: | Design decision needed |
| Has patch: | yes | Needs documentation: | yes |
| Needs tests: | yes | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
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 (8)
Change History (36)
by , 16 years ago
| Attachment: | templates_optimizations_r9076.diff added |
|---|
comment:1 by , 16 years ago
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 comment:2 by , 16 years ago
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.
comment:3 by , 16 years ago
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
comment:4 by , 16 years ago
I've found the patch doesn't works in other inner links in my project. I will search for problem.
comment:6 by , 16 years ago
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 :-)
by , 16 years ago
| Attachment: | templates_optimizations_r9076.2.diff added |
|---|
New patch that fixes a rare inheritance case.
comment:7 by , 16 years ago
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.
comment:8 by , 16 years ago
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 comment:9 by , 16 years ago
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_idunder all circumstances indjango/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 comment:10 by , 16 years ago
Also:
- The new functionality of a
dirsargument onget_templatedoesn't belong in this ticket - And I'm guessing
from_childargument slipped in there by mistake?
comment:11 by , 16 years ago
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.
comment:12 by , 16 years ago
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_idunder all circumstances indjango/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.
by , 16 years ago
| Attachment: | templates_optimizations_r9109.diff added |
|---|
New patch with SmileyChris comments fixed
comment:13 by , 16 years ago
Replying to SmileyChris:
Also:
- The new functionality of a
dirsargument onget_templatedoesn'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_childargument slipped in there by mistake?
Of course, sorry again. I've fixed that.
comment:14 by , 16 years ago
@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?
comment:15 by , 16 years ago
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)
comment:16 by , 16 years ago
| Triage Stage: | Unreviewed → 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.
comment:17 by , 16 years ago
PS: the admin.templatetags.log fix has been checked in now so that part is redundant (r9233)
by , 16 years ago
| Attachment: | templates_optimizations_r9235.diff added |
|---|
New patch without ticket #8110 patch inside (it was fixed)
comment:18 by , 16 years ago
In one of my projects, I've experimented an issue with this patch and {{ block.super }} use. I will found the error.
by , 16 years ago
| Attachment: | templates_optimizations_r9285.diff added |
|---|
New patch that fixes {{ block.super }} issue (updated to r9285)
comment:19 by , 16 years ago
| Cc: | added |
|---|
comment:20 by , 16 years ago
| Cc: | added |
|---|---|
| Owner: | changed from to |
| Status: | new → assigned |
comment:22 by , 16 years ago
| Needs documentation: | set |
|---|---|
| Needs tests: | set |
#9874 has a patch with a slightly similar implementation that implements a setting to control the cache and that includes tests.
comment:23 by , 16 years ago
| Cc: | added |
|---|---|
| Keywords: | templates cache added |
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.
by , 16 years ago
| Attachment: | #9154 - templates_optimizations.diff added |
|---|
New patch that hopefully fixes all {{ block.super }} issues
comment:24 by , 16 years ago
| Summary: | Huge improvements in templates rendering → Cache compiled templates |
|---|
As #9874 is duplicate, I closed it.
comment:25 by , 16 years ago
| Resolution: | → duplicate |
|---|---|
| Status: | new → closed |
This itself is duplicate of #6262, closing as such.
comment:26 by , 16 years ago
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.
by , 16 years ago
| Attachment: | #9154 - templates_optimizations.2.diff added |
|---|
same as the previous but with deepcopy removed
comment:27 by , 16 years ago
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.
comment:28 by , 13 years ago
| Cc: | added |
|---|---|
| Easy pickings: | unset |
| Severity: | → Normal |
| Type: | → Uncategorized |
| UI/UX: | unset |
Templates system optimization