Unnecessary usage of NodeList in ForNode (template rendering)

[anonymous]

There is some redundant rendering of nodes happening in template for loops:

In ForNode the rendering is done as follows:

• A new NodeList is initialized
• The nodes inside the for loop are rendered in the for loop's context and pushed to the nodelist
• The nodelist is rendered in the context containing the for loop

The code is at ​http://code.djangoproject.com/browser/django/trunk/django/template/defaulttags.py, lines 146, 178 and 187.

Now, the last step seems redundant and wrong. It is redundant because every node was rendered in step 2 above, so why is it needed to render the nodelist again? If there is some reason for this, there needs to be a comment explaining why. If there is no reason for this second rendering, it would be more effective to change the nodelist to a plain list and return u''.join(the_list) from the ForNode.render(). The last step is wrong, because in step 2 if a node returns something that can be rendered, then that thing will be rendered in context outside of the for loop's in step 3.

[Luke Plant]

The step is not wrong - the implementation of NodeList.render - ​http://code.djangoproject.com/browser/django/trunk/django/template/__init__.py#L796

But it may be quicker and equally correct to just do u''.join(nodelist)

[Luke Plant]

Looking at it again, I don't know if it would be correct, due to how NodeList.render uses mark_safe.

[anonymous]

I was wrong in the initial report, it is true that returning just join(nodelist) is not correct. There needs to be a couple of small changes, force_unicode and mark_safe needs to be used inside ForNode. It should be faster that way, though. There is no need for an extra for loop, and there is no need for the isinstance calls.

[Russell Keith-Magee]

Yes, it should be marginally faster, but I doubt it will be a major performance improvement. However, every little bit helps. If you provide a patch, we'll apply it.

The line numbers have gotten messed up since this was reported; permalinked line numbers are ​146
​178 and
​187 of defaulttags.py, and
​796 of template/__init__.py, which is now in base.py.

[Dmitry Trofimov]

makes what described in discussion

[Adrian Holovaty]

Just took a look at this patch. I don't see a reason for mark_safe_unicode to be a classmethod (not being a big fan of classmethods). Can you change it to be a module-level function?

[Julien Phalip]

Patch needs improvement as per Adrian's comment.

[Dmitry Trofimov]

patch improved

[Tim Graham]

Patch no longer applies cleanly.

[FunkyBob]

Simplified patch that passes all tests on current master.

[Tim Graham <timograham@…>]

In 12023887ea3e495e070b1f624078612776edf9be:

Fixed #14765 -- Removed unncessary usage of NodeList in ForNode.

Thanks traff and FunkyBob for work on the patch.