Opened 17 years ago

Last modified 17 years ago

#4523 closed

NodeList.render cleanup — at Version 1

Reported by: (removed) Owned by: Adrian Holovaty
Component: Template system Version: dev
Severity: Keywords: performance
Cc: 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 (last modified by Malcolm Tredinnick)

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?

Change History (2)

by (removed), 17 years ago

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

comment:1 by Malcolm Tredinnick, 17 years ago

Description: modified (diff)
Keywords: performance added
Triage Stage: UnreviewedDesign 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.)

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