Opened 13 years ago

Closed 11 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: dev
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 13 years ago.
makes what described in discussion
little_tweak_of_tempate_rendering2.patch (2.3 KB ) - added by Dmitry Trofimov 13 years ago.
patch improved
14765.diff (1.4 KB ) - added by FunkyBob 11 years ago.
Simplified patch that passes all tests on current master.

Download all attachments as: .zip

Change History (17)

comment:1 by Luke Plant, 13 years ago

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 by Luke Plant, 13 years ago

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

comment:3 by anonymous, 13 years ago

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 by Russell Keith-Magee, 13 years ago

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.

by Dmitry Trofimov, 13 years ago

makes what described in discussion

comment:5 by Dmitry Trofimov, 13 years ago

Has patch: set

comment:6 by Adrian Holovaty, 13 years ago

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 by anonymous, 13 years ago

Severity: Normal
Type: Bug

comment:8 by Julien Phalip, 13 years ago

Patch needs improvement: set

Patch needs improvement as per Adrian's comment.

by Dmitry Trofimov, 13 years ago

patch improved

comment:9 by Dmitry Trofimov, 13 years ago

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

comment:10 by Dmitry Trofimov, 13 years ago

Cc: Dmitry Trofimov added

comment:11 by Julien Phalip, 12 years ago

Type: BugCleanup/optimization

comment:12 by Tim Graham, 11 years ago

Patch needs improvement: set

Patch no longer applies cleanly.

by FunkyBob, 11 years ago

Attachment: 14765.diff added

Simplified patch that passes all tests on current master.

comment:13 by FunkyBob, 11 years ago

Cc: FunkyBob added
Patch needs improvement: unset

comment:14 by Tim Graham <timograham@…>, 11 years ago

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