Opened 17 years ago

Last modified 17 years ago

#4523 closed

NodeList.render cleanup — at Initial Version

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

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 (1)

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

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