Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#26095 closed Cleanup/optimization (wontfix)

Same behaviour of dict.items and defaultdict.items in DTL

Reported by: Klaus Bremer Owned by: Klaus Bremer
Component: Template system Version: master
Severity: Normal Keywords: templates dict defaultdict
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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).

Change History (9)

comment:1 Changed 5 years ago by Klaus Bremer

Owner: changed from nobody to Klaus Bremer
Status: newassigned

comment:2 Changed 5 years ago by Simon Charette

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.

comment:3 Changed 5 years ago by Klaus Bremer

Has patch: set
Needs documentation: set

comment:4 in reply to:  2 Changed 5 years ago by Klaus Bremer

Replying to 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?

comment:5 Changed 5 years ago by Tim Graham

I'm concerned the proposed PR with have an adverse effect on performance. Maybe you could see what djangobench has to say.

Last edited 5 years ago by Tim Graham (previous) (diff)

comment:6 Changed 5 years ago by Klaus Bremer

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.

comment:7 Changed 5 years ago by Tim Graham

Resolution: wontfix
Status: assignedclosed

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.

comment:8 Changed 5 years ago by Curtis Maloney

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...

comment:9 Changed 5 years ago by Klaus Bremer

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.

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