Opened 16 years ago

Closed 14 years ago

Last modified 11 years ago

#6262 closed (fixed)

Cache templates

Reported by: Chris Beaven Owned by: Michael Malone
Component: Template system Version: dev
Severity: Keywords:
Cc: mocksoul@…, lemuelf@…, Reflejo@…, mjmalone@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Templates are currently loaded and compiled from the filesystem on every get_template call. It seems like a useful feature to be able to cache these templates.

This enhancement, in my limited testing, can easily halve the time it takes to load a template.

For this to be of any real use, #5701 should be applied - otherwise templates with any stringfilter decorated filters won't be get cached.

Attachments (13)

cache_templates.diff (3.3 KB) - added by Chris Beaven 16 years ago.
test_templates_cache.tar.gz (2.7 KB) - added by Chris Beaven 16 years ago.
a small app to test speed
loader_tags.diff (9.1 KB) - added by cainmatt@… 15 years ago.
Re-written BlockNode and ExtendsNode to not update nodelist in render()
loader_tags.2.diff (10.2 KB) - added by mzzzzc 15 years ago.
revised loader_tags.diff, fixes IncludeNode BlockContext handling
loader_tags.3.diff (10.8 KB) - added by mzzzzc 15 years ago.
revised loader_tags.diff, reset BlockContext in !Template.render()
cache_templates.2.diff (28.6 KB) - added by Michael Malone 14 years ago.
template caching, refactored loaders, fixed template tags
cached_templates.3.diff (28.4 KB) - added by Michael Malone 14 years ago.
cache_templates.3.diff (30.0 KB) - added by Michael Malone 14 years ago.
cache_templates.4.diff (32.4 KB) - added by Michael Malone 14 years ago.
cache_templates.5.diff (48.0 KB) - added by Michael Malone 14 years ago.
cache_templates.6.diff (51.2 KB) - added by Michael Malone 14 years ago.
cache_templates.7.diff (51.9 KB) - added by Michael Malone 14 years ago.
t6262-rc1.diff (53.2 KB) - added by Russell Keith-Magee 14 years ago.
RC1 of cached templates patch

Download all attachments as: .zip

Change History (53)

Changed 16 years ago by Chris Beaven

Attachment: cache_templates.diff added

comment:1 Changed 16 years ago by Chris Beaven

A couple of calls I've made in the patch:

  • Templates are only cached while DEBUG is False (because if you're testing, you don't want caching getting in the way of your latest templates).
  • CACHE_TEMPLATES defaults to 600. I originally had it set to 0 (i.e. off) but I thought that people may as well benefit from it. I'd be just as comfortable with it being off by default.

Oh, and there's a comment in the patch saying "(Python 2.3 can't pickle any filter decorated with stringfilter)", since my understanding is that this will work in >2.3 after of the #5701 patch is applied.

comment:2 Changed 16 years ago by Malcolm Tredinnick

Can you please drop in some details of the performance testing you did for this. In the past, the reason template caching wasn't done is because for real world applications the speed up wasn't that significant, when measure in terms of total request time.

Changed 16 years ago by Chris Beaven

Attachment: test_templates_cache.tar.gz added

a small app to test speed

comment:3 Changed 16 years ago by Chris Beaven

Ok, I've attached an app which I added to the end of a smallish project I'm working on.

The tests use the test client to get the response, so I guess that's somewhat close to total request time. The matching url uses the direct_to_template view to display a simple template (which inherits a simple base template). The client runs 1000 gets uncached (to warm up the drive), then times 1000 cached vs 1000 uncached requests.

Granted, this is testing on a laptop with slower drive speeds than your average server, but still there is a notable difference between speeds (even with simplistic templates)

WARMING UP TO TEST FOR CACHE TEMPLATE SPEEDS
CACHED TEMPLATE...   3.9408
UNCACHED TEMPLATE... 4.4971

comment:4 Changed 16 years ago by Chris Beaven

(this is just testing with the default locmem cache, by the way - I'm not sure if other ones would be faster or slower)

comment:5 Changed 16 years ago by jim-django@…

This seems to be just cacheing the body of the template file - why would this be any better than relying on the OS's underlying buffers?
It might make more sense to check real-world profile data to see how much time is spent parsing, rather than reading, the templates.

comment:6 Changed 16 years ago by korpios

Cc: korpios@… added

comment:7 in reply to:  5 Changed 16 years ago by Antti Kaihola

Replying to jim-django@dsdd..org:

[...] how much time is spent parsing, rather than reading, the templates.

It's caching the result of get_template_from_string(), which as far as I can tell is the compiled template.

comment:8 Changed 16 years ago by Simon Greenhill <dev@…>

Triage Stage: UnreviewedDesign decision needed

comment:9 Changed 16 years ago by Chris Beaven

Owner: changed from nobody to Chris Beaven

comment:10 Changed 16 years ago by MikeH <mike@…>

I've benchmarked our applications with and without this patch. With the patch we deliver about 25% more pages per second than without.

comment:11 Changed 15 years ago by korpios

Cc: korpios@… removed

comment:12 Changed 15 years ago by Vadim Fint <mocksoul@…>

Cc: mocksoul@… added

comment:13 Changed 15 years ago by Chris Beaven

Status: newassigned

Related ticket: #9154

comment:14 Changed 15 years ago by anonymous

I've benchmarked this with an app that uses full view caching, and it seems to be the cause of a large performance DEcrease :( I'm not sure why, but without this patch I get 900page/second (using ab), with it almost halves.

comment:15 Changed 15 years ago by Chris Beaven

Could be something to do with the caching backend you're using (since this patch uses Django's caching). I don't really see how it could slow it down that much though - after the initial template cache, it should be returning your full cached view.

I find it hard to see how a one time cache can cause an ongoing half in your views if they are fully cached.

comment:16 Changed 15 years ago by anonymous

We are using memcache as the backend. I haven't had time to check it out fully yet, but it seems like it's when 'cache' gets imported on line 24 there is some work going on which slows down the response. I'll investigate further and report back.

comment:17 Changed 15 years ago by Lemuel Formacil

Cc: lemuelf@… added

comment:18 Changed 15 years ago by Martín Conte Mac Donell

Cc: Reflejo@… added

Changed 15 years ago by cainmatt@…

Attachment: loader_tags.diff added

Re-written BlockNode and ExtendsNode to not update nodelist in render()

comment:19 Changed 15 years ago by cainmatt@…

See loader_tags.diff for some changes to support caching of templates which use {% block %} and {% extends %}.

Caching only works if tags do not change their attributes in render(). render() should only set local variables and perhaps alter context.
#7501 (cycle tag) will also cause problems if templates are cached

I'm not sure which of the several "template caching" issues is the best to comment on. See also #9874 #9154

Notes on loader_tags.diff:

  • store block relationships in context rather than template nodes
  • build block dictionaries at template parse time rather than render time
  • always use get_template to load templates (not find_template_source)
  • a special block_context object is placed at context['block'] to support block overrides and {{ block.super }}.
  • TODO: reduce visibility of block_context
  • TODO: #7501

comment:20 Changed 15 years ago by Vadim Fint <mocksoul@…>

cainmatt, could you please post any speedups (in %%) you notice? Thanks.

comment:21 Changed 15 years ago by mrts

As mentioned before, #9874 and #9154 are duplicates. Closing them in favour of this one. Both of them have patches which may or may not be useful.

Changed 15 years ago by mzzzzc

Attachment: loader_tags.2.diff added

revised loader_tags.diff, fixes IncludeNode BlockContext handling

Changed 15 years ago by mzzzzc

Attachment: loader_tags.3.diff added

revised loader_tags.diff, reset BlockContext in !Template.render()

comment:22 Changed 15 years ago by mzzzzc

Vadim,

It would be best for you to test speedups on your own application. Normally template parsing would only be a small part of overall application time (~<20%).

The loader_tags.diff changes by themselves would not result in a speedup. They are necessary but not sufficient for caching templates.

I have implemented a very simple dict lookup in template.loader.get_template() similar to #9154.

The reason I wanted to cache templates is that I have custom tags which do some processing during parsing which I don't want to repeat all the time.

A very rough test of one of my pages yielded:
Requests / sec
18 Django-1.0.2
24 template cache on (33% improvement)

Changed 14 years ago by Michael Malone

Attachment: cache_templates.2.diff added

template caching, refactored loaders, fixed template tags

comment:23 Changed 14 years ago by Michael Malone

Cc: mjmalone@… added

I've attached a diff (cache_templates.2.diff) that's sort of a synthesis of patches and ideas from comments here and in #9154 and #9874. I've refactored a bunch of the template loading stuff to make it easier to extend. With this patch you can optionally return a compiled template from a template loader, this allows you to implement template caching in a template loader. I've also added a parser_context stack to the Context object. This can be used by template tags to store state (instead of storing it in instance variables) and makes creating thread-safe template tags a lot easier. I've gone ahead and fixed block, extends, and cycle (thanks to the prior work on this from cainmatt and mzzzzc).

Thoughts & comments more than welcome. I want to try to get some version of this into 1.2 since, by my measurements, it's a big win for apps that have complex templates.

comment:24 Changed 14 years ago by Chris Beaven

Owner: changed from Chris Beaven to Michael Malone
Status: assignednew

Looking good. I've reassigned the ticket to you for now.

Couple of thoughts from a quick (non-comprehensive) review:

  • Good job on the parser context idea. My thoughts were to just create a render-local "globals" - do you think the multi-level idea of context is really necessary?
  • You should abstract the common elements of ParserContext and Context to a BaseContext class.
  • In the loaders, shouldn't you keep the top level function get_template_sources around for backwards compatibility too?
  • In BaseLoader, you named the function load_template_from_source when it should be load_template_source
  • In loaders.py, you're renaming the function find_template_source to find_template - is it worth it for the backwards incompatibility?

comment:25 in reply to:  24 Changed 14 years ago by Michael Malone

Replying to SmileyChris:

Awesome feedback SmileyChris, thanks.

  • Good job on the parser context idea. My thoughts were to just create a render-local "globals" - do you think the multi-level idea of context is really necessary?

I started with basically just a dictionary, but I realized that each template needs a new block context when it's rendered, otherwise recursive rendering (i.e., included templates) breaks. When I thought about it I figured it probably made sense to start each template render with a "fresh" context. It made the block context implementation easier, and I couldn't think of any use cases where you wouldn't want this behavior, so I went with it. I'm definitely flexible on this matter though if anyone has a better idea!

  • You should abstract the common elements of ParserContext and Context to a BaseContext class.

Definitely. Fixed in new version.

  • In the loaders, shouldn't you keep the top level function get_template_sources around for backwards compatibility too?

Possibly... I guess it is technically public interface (though I doubt it's used much in practice). I guess it wouldn't hurt to implement it the same way load_template_source is implemented (except that we'd basically be committing to supporting this functionality indefinitely, which may make future changes more difficult).

  • In BaseLoader, you named the function load_template_from_source when it should be load_template_source

Good catch, fixed in new version.

  • In loaders.py, you're renaming the function find_template_source to find_template - is it worth it for the backwards incompatibility?

Another good question. I'm not sure one way or the other. If people are using this in practice, then it's probably worth keeping it. I renamed it since it's no longer returning template "source" all the time (as you probably guessed).

Changed 14 years ago by Michael Malone

Attachment: cached_templates.3.diff added

Changed 14 years ago by Michael Malone

Attachment: cache_templates.3.diff added

Changed 14 years ago by Michael Malone

Attachment: cache_templates.4.diff added

comment:26 Changed 14 years ago by Michael Malone

New patch addresses a couple of failing tests. There wasn't really anything broken in the implementation, but some of the tests made assumptions that were no longer valid due to small changes in the internal API. Where possible/practical I changed the API rather than the tests.

comment:27 Changed 14 years ago by Chris Beaven

The problem with giant patches is that there's the temptation to "fix" other things. An example here is changing the ordering of the context layers. Now this *may* be a valid change, but there is a separate issue open for this, and you don't need to do it to make your patch work.

Even if it's not a public interface, I have seen people using context[-1]['some_global'] = yay_for_hacking_internals. So "fixing" not critical things just hurts getting the ticket committed (due to backwards incompatibilty concerns).

comment:28 Changed 14 years ago by Michael Malone

Eh, I don't care one way or the other about that particular issue. I reversed the order in BlockContext (on Alex Gaynor's recommendation) because it's more efficient. Then I reversed the order in Context when I consolidated the two classes and made BaseContext.

I'm all for maintaining backwards compatibility wherever possible, but I think in this particular case you're messing with something that's pretty clearly an implementation detail and you shouldn't expect it to remain the same across releases. That said, if it became an issue that blocked this ticket from a merge I'd certainly change it.

comment:29 Changed 14 years ago by Chris Beaven

Actually, my bad - I thought there was another ticket covering this, but maybe I was just having an IRC conversation, or maybe it was the voices in my head.

Still, it is a separate issue really. But since you've already got Alex's recommendation on it I guess there's not too much worry (I get told off from Malcolm for combining issues, it must be rubbing off)

comment:30 Changed 14 years ago by Michael Malone

Ha, no problem. I just went ahead and did it since I was refactoring the Context code anyways. If anyone has any issues with this, or can show a use case where poking at context.dicts is actually necessary (or just an example of somewhere someone does it in the wild) please do share.

Changed 14 years ago by Michael Malone

Attachment: cache_templates.5.diff added

comment:31 Changed 14 years ago by Michael Malone

New patch addresses some issues that Alex Gaynor had, renames parser_context to render_context, adds docs, etc. No big changes, basically just a refinement on the previous version.

comment:32 in reply to:  31 ; Changed 14 years ago by Antti Kaihola

Replying to mmalone:

New patch

On line 538 of docs/howto/custom-template-tags.txt, you probably mean

There may be multiple ``CycleNode``s in a given template

instead of

There may be ``CycleNode``'s in a given template

comment:33 in reply to:  32 ; Changed 14 years ago by Michael Malone

Replying to akaihola:

Replying to mmalone:

New patch

On line 538 of docs/howto/custom-template-tags.txt, you probably mean

There may be multiple ``CycleNode``s in a given template

instead of

There may be ``CycleNode``'s in a given template

Yea, that is what I meant, but if you don't have an apostrophe reST/sphinx poops its pants and doesn't format it correctly... so I punted. Nice bike shed though if you wanna figure out how to fix it ;). Honestly though, I do appreciate the thorough editorial review.

comment:34 in reply to:  33 ; Changed 14 years ago by Michael Malone

Replying to mmalone:

Replying to akaihola:

Replying to mmalone:

New patch

On line 538 of docs/howto/custom-template-tags.txt, you probably mean

There may be multiple ``CycleNode``s in a given template

instead of

There may be ``CycleNode``'s in a given template

Yea, that is what I meant, but if you don't have an apostrophe reST/sphinx poops its pants and doesn't format it correctly... so I punted. Nice bike shed though if you wanna figure out how to fix it ;). Honestly though, I do appreciate the thorough editorial review.

Er, I changed it so the 's' is inside the backticks, which is apparently how it's done elsewhere in the docs. Woot!

Changed 14 years ago by Michael Malone

Attachment: cache_templates.6.diff added

comment:35 Changed 14 years ago by Michael Malone

New patch adds deprecation warnings to load_template_source functions and mentions this API in the internals/deprecation docs. I've written a replacement implementation of find_template_source for backwards compatibility. Russ suggested an alternative approach for specifying loaders, where each loader can be a tuple and t[0] is the loader module and t[1:] are args. This is useful because you no longer have to import and instantiate the loader in your settings.py, so I went ahead and implemented it. Finally, fixed a little bug in the Context.get implementation.

comment:36 in reply to:  34 ; Changed 14 years ago by Antti Kaihola

Replying to mmalone:

Er, I changed it so the 's' is inside the backticks, which is apparently how it's done elsewhere in the docs. Woot!

Actually, I meant to point out the missing word "multiple" (or "several").

comment:37 in reply to:  36 Changed 14 years ago by Michael Malone

Replying to akaihola:

Replying to mmalone:

Er, I changed it so the 's' is inside the backticks, which is apparently how it's done elsewhere in the docs. Woot!

Actually, I meant to point out the missing word "multiple" (or "several").

Sorry, good call. Will fix.

Changed 14 years ago by Michael Malone

Attachment: cache_templates.7.diff added

Changed 14 years ago by Russell Keith-Magee

Attachment: t6262-rc1.diff added

RC1 of cached templates patch

comment:38 Changed 14 years ago by Russell Keith-Magee

Resolution: fixed
Status: newclosed

(In [11862]) Fixed #6262 -- Added a cached template loader, and modified existing template loaders and tag to be cacheable. Thanks to Mike Malone for the patch.

comment:39 Changed 11 years ago by Ramiro Morales <cramm0@…>

In 826d9de00e74a53d7cc65fcb2aaa5ccdf33674ab:

Fixed #19729 -- Removed leftover refactoring helper variables.

Thanks chrismedrela for the report.

Refs #6262, 44b9076 and 4d94c0c.

comment:40 Changed 11 years ago by Ramiro Morales <cramm0@…>

In 6f29147488e14555ae2e7c56f0f74996010e97e3:

[1.5.x] Fixed #19729 -- Removed leftover refactoring helper variables.

Thanks chrismedrela for the report.

Refs #6262, 44b9076 and 4d94c0c.

826d9de00e74a53d7cc65fcb2aaa5ccdf33674ab from master.

Note: See TracTickets for help on using tickets.
Back to Top