Code

Opened 6 years ago

Closed 5 years ago

Last modified 3 years ago

#9154 closed Uncategorized (duplicate)

Cache compiled templates

Reported by: msaelices Owned by:
Component: Template system Version: master
Severity: Normal Keywords: templates, cache
Cc: lemuelf@…, Reflejo@…, german.mb@…, Kronuz 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)

templates_optimizations_r9076.diff (4.3 KB) - added by msaelices 6 years ago.
Templates system optimization
templates_optimizations_r9076.2.diff (4.6 KB) - added by msaelices 6 years ago.
New patch that fixes a rare inheritance case.
templatebench.py (1.6 KB) - added by msaelices 6 years ago.
A trivial bench
templates_optimizations_r9109.diff (4.5 KB) - added by msaelices 6 years ago.
New patch with SmileyChris comments fixed
templates_optimizations_r9235.diff (3.6 KB) - added by msaelices 6 years ago.
New patch without ticket #8110 patch inside (it was fixed)
templates_optimizations_r9285.diff (4.5 KB) - added by msaelices 6 years ago.
New patch that fixes {{ block.super }} issue (updated to r9285)
#9154 - templates_optimizations.diff (5.5 KB) - added by Kronuz 5 years ago.
New patch that hopefully fixes all {{ block.super }} issues
#9154 - templates_optimizations.2.diff (5.4 KB) - added by Kronuz 5 years ago.
same as the previous but with deepcopy removed

Download all attachments as: .zip

Change History (36)

Changed 6 years ago by msaelices

Templates system optimization

comment:1 Changed 6 years ago by msaelices

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

comment:2 follow-up: Changed 6 years ago 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.

comment:3 in reply to: ↑ 2 Changed 6 years ago 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

comment:4 Changed 6 years ago by msaelices

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

comment:5 Changed 6 years ago by msaelices

Problem is in block inheritance.

comment:6 Changed 6 years ago 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 :-)

Changed 6 years ago by msaelices

New patch that fixes a rare inheritance case.

Changed 6 years ago by msaelices

A trivial bench

comment:7 Changed 6 years ago 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.

comment:8 Changed 6 years ago 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.

comment:9 follow-up: Changed 6 years ago 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 :)

comment:10 follow-up: Changed 6 years ago 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?

comment:11 Changed 6 years ago 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.

comment:12 in reply to: ↑ 9 Changed 6 years ago 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.

Changed 6 years ago by msaelices

New patch with SmileyChris comments fixed

comment:13 in reply to: ↑ 10 Changed 6 years ago 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.

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

comment:15 Changed 6 years ago 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)

comment:16 Changed 6 years ago by mtredinnick

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

comment:17 Changed 6 years ago by SmileyChris

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

Changed 6 years ago by msaelices

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

comment:18 Changed 6 years ago by msaelices

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

Changed 6 years ago by msaelices

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

comment:19 Changed 6 years ago by lemuelf

  • Cc lemuelf@… added

comment:20 Changed 5 years ago by Reflejo

  • Cc Reflejo@… added
  • Owner changed from msaelices to Reflejo
  • Status changed from new to assigned

comment:21 Changed 5 years ago by Reflejo

  • Owner Reflejo deleted
  • Status changed from assigned to new

ups.

comment:22 Changed 5 years ago by ramiro

  • 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 Changed 5 years ago by Kronuz

  • Cc german.mb@… 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.

Changed 5 years ago by Kronuz

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

comment:24 Changed 5 years ago by mrts

  • Summary changed from Huge improvements in templates rendering to Cache compiled templates

As #9874 is duplicate, I closed it.

comment:25 Changed 5 years ago by mrts

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

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

comment:26 Changed 5 years ago 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.

Changed 5 years ago by Kronuz

same as the previous but with deepcopy removed

comment:27 Changed 5 years ago 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.

comment:28 Changed 3 years ago by Kronuz

  • Cc Kronuz added
  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset

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.