Django

Code

Ticket #4523 (closed: duplicate)

Opened 1 year ago

Last modified 1 year ago

NodeList.render cleanup

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: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description (Last modified by mtredinnick)

Tucked away in NodeList.render is a fun little isinstance check, basically

if isinstance(node, Node):
  bits.append(self.render_node(node, context))
else:
  bits.append(node)

With the render method being a hook method for derivative classes to intercept exceptions, and add some debugging info in; two limiting points to scaling there
1) Because ForNode uses it to store strings, the isinstance is required- no other code uses it for this functionality, making me think ForNode shouldn't be forcing NodeList to do something odd, should do the job itself
2) Because DebugNodeList overrides render_node, instead of render, an extra func call is involved for every node; again, single usage slows down the common usage.

So... question is, why should ForNode use it? Have yet to find any comments/explanation code wise, only thing I can *guess* is that someone was trying to extend DebugNodeList.render_node behaviour down through a for; as said, this hurts the common case.

Realize the first reaction is likely going to be another "micro-optimization is evil" comment; thing is, while it's not a huge gain like say removing dispatch.send invocations from Models.*, it's a constant waste occuring in any NodeList.render (which are common enough). Usual usage scenario I'm abusing for testing pegs it as a constant 1.4s hit against trunk 64s (26s if using my misc patches sitting in tickets); might not seem like much, but it's yet another spot where it's spinning it's wheels without reason.

So... why is ForNode abusing NodeList here? Oversight? Worth preserving?

Attachments

NodeList-cleanup.patch (2.7 kB) - added by Brian Harring <ferringb@gmail.com> on 06/10/07 09:22:12.
NodeList/ForNode? cleanup; ForNode? uses a list directly, remove render_node from NodeList/DebugNodeList?, instead moving the override into DebugNodeList? and simplifying NodeList?.render

Change History

06/10/07 09:22:12 changed by Brian Harring <ferringb@gmail.com>

  • attachment NodeList-cleanup.patch added.

NodeList/ForNode? cleanup; ForNode? uses a list directly, remove render_node from NodeList/DebugNodeList?, instead moving the override into DebugNodeList? and simplifying NodeList?.render

06/11/07 04:15:14 changed by mtredinnick

  • description changed.
  • needs_better_patch changed.
  • needs_tests changed.
  • keywords set to performance.
  • needs_docs changed.
  • stage changed from Unreviewed to Design decision needed.

One extra use-case to consider here is what other template tags might be using this. I don't know the answer to that, but changing the API here does have an effect on third-party template tags, so we have to weigh that up. For that reason, it's a backwards-incompatible change.

I'm +1 on this. The extra interface provided by NodeList (only get_nodes_by_type()) is easy to replicate if some other tags are using NodeList instead of a normal list for that purpose.

Will leave it for a little while to give Adrian or Jacob a chance to chime in, but it's worth doing, I think.

(Adding "performance" keyword as per Jacob's suggestion.)

06/16/07 20:31:12 changed by mtredinnick

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

We'll take #4565 and this patch is incorporated into that change.


Add/Change #4523 (NodeList.render cleanup)




Change Properties
Action