Django

Code

Ticket #4565 (closed: fixed)

Opened 1 year ago

Last modified 1 year ago

template rendering should yield items, not concat strings at each level

Reported by: Brian Harring <ferringb@gmail.com> Assigned to: adrian
Milestone: Component: Template system
Version: SVN Keywords: performance
Cc: Triage Stage: Design decision needed
Has patch: 0 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

Given following template-

{% for x in blah %}
  {% for y in x %}
    {% bunch o nodes %{
  {% endfor %}
{% endfor %}

The way the template rendering subsystem works, each 'bunch o nodes' is added onto a list w/in 'for y', then collapsed into a string, and returned to 'for x'; which builds up a list, collapses it down into into a string, and returns it. Problems follow:

* wind up continually generating larger and larger strings as data is added on, percolating it's way up to the actual template.render. This isn't totally friendly to performance (specifically has a knack for forcing the alloc pool to expand repeatedly).

* Data is already ready to be sent, but it's locked away in the template- basically, template rendering tries to build a final string of all contents- until that's finished nothing is sent

* Said "final string of all content" means that django winds up being more memory hungry, pretty much without any reason.

A backwards compatible solution is the introduction of an iter_render method to Node, that yields finalized chunks as they're available- this doesn't mean yielding each char as it's available, means (for a ForNode? for example), yielding each chunk of data for iteration; via this approach, Template.iter_render has chunks of data percolate up as they're available, instead of just one big ass string at the end.

Attached is a patch implementing iter_render, and converting existing tags/templates/involved codepaths over to iteration where sane; some node rendering still collapse internally (filter expressions primarily, although will be targeting those later). This patch includes the patch from ticket #4523 (isinstance usage there gets ugly when a lot of items are passing through it).

Final other change worth noting- changes the base http so that it's no longer flushing after every single write during content dumping. Reason behind this should be fairly obvious- fine if you're writing a single item, sucks if you're writing a few hundred.

One point of potential contention is the change to Node;

class Node(object):
  # misc fluff
  def render(self, context):
    return ''.join(self.iter_render(context))

  def iter_render(self.context):
    return (self.render(context),)

In other words, the default Node definition for rendering will trigger a RuntimeError? on execution, going recursive. The reason for this choice is backwards compatibility- all custom Node derivatives at this point define render, thus iter_render has to defer to it if iter_render is default. Lack of overriding can probably be detected (and explode up front) via a metaclass trick, although open to suggestions.

The cons are pretty well covered above; Pros are

* instant results; as long as you don't have any tags/nodes that require collapsing their subtree down into a single string, data is sent out immediately. Rather nice if you have a slow page- you start get chunks of the page immediately, rather then having to wait for the full 10s/whatever time for the page to render.

* Far less abusive on memory; usual 'spanish inquisition' test (term coined by mtreddinick, but it works), reduction from 84m to 71m for usage at the end of rendering. What I find rather interesting about that reduction is that the resultant page is 6.5 mb; the extra 7mb I'm actually not sure where the reduction comes from (suspect there is some copy idiocy somewhere forcing a new string)- alternatively, may just be intermediate data hanging around, since I've been working with this code in one form or another for 3 months now, and still haven't figured out the 'why' for that diff.

* Surprisingly, at least for the test cases I have locally, I'm not seeing a noticable hit on small pages, nothing above background noise at the very least- larger pages, seeing a mild speed up (which was expected- 4-5%).

Patch is attached, bzr branch should it bitrot few months down the line, http://pkgcore.org/~ferringb/django/iter-render . Will be bugging the ml about this ticket also, since I'd expect folks will want to chime in; finally, docs aren't updated in this patch- didn't see the point in doing it till I was sure folks would go for the change.

Attachments

template-iter-render-r3369.patch (38.3 kB) - added by Brian Harring <ferringb@gmail.com> on 06/14/07 14:01:12.

Change History

06/14/07 14:01:12 changed by Brian Harring <ferringb@gmail.com>

  • attachment template-iter-render-r3369.patch added.

06/15/07 04:32:53 changed by Simon G. <dev@simon.net.nz>

  • needs_better_patch changed.
  • stage changed from Unreviewed to Design decision needed.
  • needs_tests changed.
  • needs_docs changed.

Hi Brian - this type of detailed enhancement is best raised on the Django-Developers mailing list if you haven't done so already.

(follow-up: ↓ 3 ) 06/17/07 02:11:38 changed by mtredinnick

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

(In [5482]) Fixed #4565 -- Changed template rendering to use iterators, rather than creating large strings, as much as possible. This is all backwards compatible. Thanks, Brian Harring.

(in reply to: ↑ 2 ) 06/18/07 09:10:46 changed by boobsd@gmail.com

  • status changed from closed to reopened.
  • component changed from Template system to django.newforms.
  • summary changed from template rendering should yield items, not concat strings at each level to KeyError in sessions with newforms.
  • keywords changed from performance to sessions.
  • has_patch deleted.
  • resolution deleted.

Replying to mtredinnick:

(In [5482]) Fixed #4565 -- Changed template rendering to use iterators, rather than creating large strings, as much as possible. This is all backwards compatible. Thanks, Brian Harring.

After [5482] appeared KeyError? in django/contrib/sessions/middleware.py in _getitem_, line 20

06/18/07 09:24:40 changed by boobsd@gmail.com

Because the upload does not work, this is link to sample django project with KeyError?: http://vega-holding.ru/media/r5482.tar.gz

06/18/07 18:55:16 changed by mtredinnick

  • status changed from reopened to closed.
  • resolution set to fixed.

There's no reason to reopen this ticket when you've also opened #4613 for the same problem.

06/18/07 19:00:58 changed by Brian Harring <ferringb@gmail.com>

  • keywords changed from sessions to performance.
  • component changed from django.newforms to Template system.
  • summary changed from KeyError in sessions with newforms to template rendering should yield items, not concat strings at each level.

reverting changes for searching reasons.

06/22/07 02:15:05 changed by mtredinnick

(In [5511]) Backed out the changes in [5482] for a bit whilst some more investigation into side-effects is done. Refs #4565.

07/14/07 04:45:53 changed by Simon G. <dev@simon.net.nz>

#4650 contains another side effect of this (stale db connections hanging around under mod_python).


Add/Change #4565 (template rendering should yield items, not concat strings at each level)




Change Properties
Action