[Django] #26095: Same behaviour of dict.items and defaultdict.items in DTL
Django
-------------------------------------+-------------------------------------
Reporter: kbr | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Template system | Version: master
Severity: Normal | Keywords: templates dict
| defaultdict
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Recently I've struggled over the defaultdict-issue mentioned here
[https://docs.djangoproject.com/en/1.9/ref/templates/language/#variables].
In my opinion it may be more intuitive if dictionaries and defaultdicts
behave the same (as in plain Python).
--
Ticket URL: <https://code.djangoproject.com/ticket/26095>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: master
Keywords: templates dict | Triage Stage:
defaultdict | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => kbr
* needs_docs: => 0
* status: new => assigned
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26095#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: master
Keywords: templates dict | Triage Stage:
defaultdict | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by charettes):
Do you have an idea of how this could be solved without breaking backward
compatibility?
See #25574 for the discussion that added this mention in the docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/26095#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: master
Keywords: templates dict | Triage Stage:
defaultdict | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_docs: 0 => 1
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26095#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: master
Keywords: templates dict | Triage Stage:
defaultdict | Unreviewed
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by kbr):
Replying to [comment:2 charettes]:
> Do you have an idea of how this could be solved without breaking
backward compatibility?
>
> See #25574 for the discussion that added this mention in the docs.
Thank you for the fast reply! I've read about #25574 before but don't see
the point about a backward incompatibility. Existing templates should work
as before and a key 'items' will still mask the object-method. But
defaultdicts don't have to get typecasted to dicts to work like dicts in
the DTL anymore. Or do I miss something?
--
Ticket URL: <https://code.djangoproject.com/ticket/26095#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: master
Keywords: templates dict | Triage Stage:
defaultdict | Unreviewed
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
I'm concerned the proposed [https://code.djangoproject.com/ticket/26095
PR] with have an adverse effect on performance. Maybe you could see what
[https://github.com/django/djangobench djangobench] has to say.
--
Ticket URL: <https://code.djangoproject.com/ticket/26095#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: master
Keywords: templates dict | Triage Stage:
defaultdict | Unreviewed
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by kbr):
Well, the isinstance()-call is about 4 times slower than the direct access
(d[bit] alone takes about 98 ns. (on a dict with a single entry)).
{{{
%%timeit
if isinstance(d, defaultdict):
pass
res = d[bit]
1000000 loops, best of 3: 378 ns per loop
}}}
Whether this is a real bottleneck depends how often Variable.resolve gets
called and - may be more important - other time consuming parts of the
application. Will try djangobench the next days. Thank you for the review
so far.
--
Ticket URL: <https://code.djangoproject.com/ticket/26095#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Template system | Version: master
Keywords: templates dict | Triage Stage:
defaultdict | Unreviewed
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
Comment:
In #django-dev, FunkyBob says, "My rule is - never pass defaultdict to a
template. In the [fairly simplistic] profiling I've run on templates,
isinstance is _the_ biggest cause of time in template rendering."
Feel free to reopen if you come up with an alternate implementation.
--
Ticket URL: <https://code.djangoproject.com/ticket/26095#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Template system | Version: master
Keywords: templates dict | Triage Stage:
defaultdict | Unreviewed
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by funkybob):
Further to the above:
- remember this is an isinstance test for every step of every lookup.
- what about when someone subclasses dict and gives it a __missing__
method? Same problem, won't be solved
- precedent : once we special case handling of one type, I worry more will
come...
--
Ticket URL: <https://code.djangoproject.com/ticket/26095#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Template system | Version: master
Keywords: templates dict | Triage Stage:
defaultdict | Unreviewed
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by kbr):
Some final thoughts, but at the very beginning: I'm in no way annoyed that
the patch doesn't get merged!
As funkybob mentioned, isinstance is called for every step of every
lookup. So a template like this will cause 4 additional calls:
{{{
{% for k, v in d.items %}{{ k }}:{{ v }}{% endfor %}
}}}
I have set up a tiny script (with 1000 times the above snippet) for some
simple profiling to get a feeling for the impact:
{{{
from django.template.base import Context
from django.template.engine import Engine
s = '{% for k, v in d.items %}{{ k }}:{{ v }}{% endfor %}'
d = {'one': [1]}
n = 1000
s *= n
engine = Engine()
t = engine.from_string(s)
context = Context({'d': d,})
#res = t.render(context)
}}}
The last line is commented out to get the overhead for preparing the
template and the context. Calling 'python -m cProfile -s cumulativ
_profile.py' now gives a fairly large output:
{{{
322295 function calls (316792 primitive calls) in 0.420 seconds
...
20469 0.005 0.000 0.005 0.000 {built-in method isinstance}
}}}
isinstance() gets called 20469 times. Most of these calls are because of
's *= n'. Uncommenting the last line now renders with the unpatched
version:
{{{
464331 function calls (455828 primitive calls) in 0.543 seconds
...
50472 0.012 0.000 0.012 0.000 {built-in method isinstance}
}}}
shows that isinstance gets called excessively and takes about 7 ms
additional runtime.
Rendering with the patched version should produce even 4k more calls:
{{{
468330 function calls (459827 primitive calls) in 0.517 seconds
...
54471 0.013 0.000 0.013 0.000 {built-in method isinstance}
}}}
This is the case and runs 1 ms longer.
To prepare a context for 4k lookups it may take some additional time for
accessing a db and the template may not come from a prepared string but
from another io-channel. All in all I think that the impact of the patch
is insignificant.
@funkybob: you further mentioned a precedent. Well, that may be a point.
IMHO it's quiet ok to expect as least surprise using dicts and
defaultdicts as these datatypes are builtins resp. from the standard
library. One can not assume that individual modified classes derived from
these types will have no unexpected behaviour in the way the DTL-lookup
works. But that's just my opinion and there may be other ones.
--
Ticket URL: <https://code.djangoproject.com/ticket/26095#comment:9>