Changes between Initial Version and Version 1 of Ticket #4523


Ignore:
Timestamp:
Jun 11, 2007, 4:15:14 AM (17 years ago)
Author:
Malcolm Tredinnick
Comment:

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.)

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #4523

    • Property Keywords performance added
    • Property Triage Stage Unreviewed → Design decision needed

  • Ticket #4523 – Description

    initial v1  
    1 Tucked away in NodeList.render is a fun little isinstance check, basically
     1Tucked away in !NodeList.render is a fun little isinstance check, basically
    22{{{
    33if isinstance(node, Node):
    … …  
    88
    99With 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[[BR]]
    10 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[[BR]]
    11 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.
     101) 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[[BR]]
     112) 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.
    1212
    13 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.
     13So... 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.
    1414
    15 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.
     15Realize 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.
    1616
    17 So... why is ForNode abusing NodeList here?  Oversight?  Worth preserving?
     17So... why is !ForNode abusing !NodeList here?  Oversight?  Worth preserving?
Back to Top