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?
NodeList/ForNode cleanup; ForNode uses a list directly, remove render_node from NodeList/DebugNodeList, instead moving the override into DebugNodeList and simplifying NodeList.render