Opened 14 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)
Change History (17)
comment:1 by , 14 years ago
comment:2 by , 14 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 , 14 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 , 14 years ago
milestone: | 1.3 |
---|---|
Triage Stage: | Unreviewed → 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.
by , 14 years ago
Attachment: | little_tweak_of_tempate_rendering.patch added |
---|
makes what described in discussion
comment:5 by , 14 years ago
Has patch: | set |
---|
comment:6 by , 14 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 , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:8 by , 14 years ago
Patch needs improvement: | set |
---|
Patch needs improvement as per Adrian's comment.
comment:9 by , 13 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | unset |
UI/UX: | unset |
comment:10 by , 13 years ago
Cc: | added |
---|
comment:11 by , 13 years ago
Type: | Bug → Cleanup/optimization |
---|
by , 11 years ago
Attachment: | 14765.diff added |
---|
Simplified patch that passes all tests on current master.
comment:13 by , 11 years ago
Cc: | added |
---|---|
Patch needs improvement: | unset |
comment:14 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
The step is not wrong - the implementation of
NodeList.render
- http://code.djangoproject.com/browser/django/trunk/django/template/__init__.py#L796But it may be quicker and equally correct to just do
u''.join(nodelist)