Code

Opened 7 years ago

Closed 7 years ago

#4523 closed (duplicate)

NodeList.render cleanup

Reported by: (removed) Owned by: adrian
Component: Template system Version: master
Severity: Keywords: performance
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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

NodeList-cleanup.patch (2.7 KB) - added by (removed) 7 years ago.
NodeList/ForNode cleanup; ForNode uses a list directly, remove render_node from NodeList/DebugNodeList, instead moving the override into DebugNodeList and simplifying NodeList.render

Download all attachments as: .zip

Change History (3)

Changed 7 years ago by (removed)

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 Changed 7 years ago by mtredinnick

  • Description modified (diff)
  • Keywords performance added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage 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.)

comment:2 Changed 7 years ago by mtredinnick

  • Resolution set to duplicate
  • Status changed from new to closed

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.