Code

Opened 3 years ago

Closed 8 months 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: traff, 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 traff 3 years ago.
makes what described in discussion
little_tweak_of_tempate_rendering2.patch (2.3 KB) - added by traff 3 years ago.
patch improved
14765.diff (1.4 KB) - added by FunkyBob 8 months ago.
Simplified patch that passes all tests on current master.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 3 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 3 years ago by lukeplant

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

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

  • milestone 1.3 deleted
  • Triage Stage changed from Unreviewed to Accepted

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 3 years ago by traff

makes what described in discussion

comment:5 Changed 3 years ago by traff

  • Has patch set

comment:6 Changed 3 years ago by adrian

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

  • Severity set to Normal
  • Type set to Bug

comment:8 Changed 3 years ago by julien

  • Patch needs improvement set

Patch needs improvement as per Adrian's comment.

Changed 3 years ago by traff

patch improved

comment:9 Changed 3 years ago by traff

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

comment:10 Changed 3 years ago by traff

  • Cc traff added

comment:11 Changed 2 years ago by julien

  • Type changed from Bug to Cleanup/optimization

comment:12 Changed 9 months ago by timo

  • Patch needs improvement set

Patch no longer applies cleanly.

Changed 8 months ago by FunkyBob

Simplified patch that passes all tests on current master.

comment:13 Changed 8 months ago by FunkyBob

  • Cc FunkyBob added
  • Patch needs improvement unset

comment:14 Changed 8 months ago by Tim Graham <timograham@…>

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

In 12023887ea3e495e070b1f624078612776edf9be:

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

Thanks traff and FunkyBob for work on the patch.

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.