Opened 6 years ago

Closed 3 years ago

#14765 closed Cleanup/optimization (fixed)

Unnecessary usage of NodeList in ForNode (template rendering)

Reported by: anonymous Owned by: nobody
Component: Template system Version: master
Severity: Normal Keywords: ForNode render
Cc: Dmitry Trofimov, FunkyBob Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (3)

little_tweak_of_tempate_rendering.patch (1.7 KB) - added by Dmitry Trofimov 6 years ago.
makes what described in discussion
little_tweak_of_tempate_rendering2.patch (2.3 KB) - added by Dmitry Trofimov 5 years ago.
patch improved
14765.diff (1.4 KB) - added by FunkyBob 3 years ago.
Simplified patch that passes all tests on current master.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by 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)

comment:2 Changed 6 years ago by Luke Plant

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

comment:3 Changed 6 years ago by 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.

comment:4 Changed 6 years ago by Russell Keith-Magee

milestone: 1.3
Triage Stage: UnreviewedAccepted

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.

Changed 6 years ago by Dmitry Trofimov

makes what described in discussion

comment:5 Changed 6 years ago by Dmitry Trofimov

Has patch: set

comment:6 Changed 6 years ago by 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?

comment:7 Changed 6 years ago by anonymous

Severity: Normal
Type: Bug

comment:8 Changed 6 years ago by Julien Phalip

Patch needs improvement: set

Patch needs improvement as per Adrian's comment.

Changed 5 years ago by Dmitry Trofimov

patch improved

comment:9 Changed 5 years ago by Dmitry Trofimov

Easy pickings: unset
Patch needs improvement: unset
UI/UX: unset

comment:10 Changed 5 years ago by Dmitry Trofimov

Cc: Dmitry Trofimov added

comment:11 Changed 5 years ago by Julien Phalip

Type: BugCleanup/optimization

comment:12 Changed 3 years ago by Tim Graham

Patch needs improvement: set

Patch no longer applies cleanly.

Changed 3 years ago by FunkyBob

Attachment: 14765.diff added

Simplified patch that passes all tests on current master.

comment:13 Changed 3 years ago by FunkyBob

Cc: FunkyBob added
Patch needs improvement: unset

comment:14 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 12023887ea3e495e070b1f624078612776edf9be:

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

Thanks traff and FunkyBob for work on the patch.

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