Skip to content

WIP - template panel context issues #1076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

kezabelle
Copy link
Contributor

As I've now encountered an issue myself where the #910 changes caused problems, possibly related to some of the others, probably I should try and fix them all up.

As a short reminder, after template based widgets came along, and before #942 landed, I tried to ensure the number of calls to pprint.pformat was kept to a minimum, as it is an expensive calculation (#933).

In doing so, the call changed from: try: ... pformat(value); except: ... to use force_text instead of pformat. This was, in hindsight, a mistake, mostly around usages of Form instances.

Because __str__ on a Form will by default render the form, it has a number of side-effects:


In an ideal world, we could just change to repr (#725) instead of force_text, as this is similarly lightning fast. Instead, to err on the side of caution, I've opted for calling pprint.saferepr instead, which is protected against recursive data structures.
This has the benefit of still being faster than pformat (as far as I can tell) and avoids a certain class of problems the other options bring. But it's much slower than calling repr or force_text
directly.


Marking as WIP because there's a number of other issues (#989, #1024, #1053, #725) are related, and one option is just to undo the changes I made to avoid too many pformat calls. Also there's no tests ;)

…-beyond triggering a DB query.

This is most prevalent with objects like Forms, whose default __str__ renders the form, and causes things like full_clean to take place when they otherwise wouldn't.
In an ideal world we'd use repr() directly, as it is SUPER fast by comparison. saferepr offers a happier middleground between repr() and pformat().
@kezabelle
Copy link
Contributor Author

Rough and ready benchmark of the different options:

from datetime import datetime
from pprint import pformat, saferepr
from django.utils.encoding import force_text

todump = {
    '1': 1,
    '2': '1',
    '3': None,
    '4': datetime.now(),
    '5': None,
    '6': object,
    '7': set(),
    '8': frozenset(),
    '9': [],
    '10': (),
}
if __name__ == '__main__':
    import timeit
    print(timeit.timeit('func(todump)', globals={'todump': todump, 'func': force_text}, number=50000))

swap out func for which-ever. Order, in lowest time to completion, should hopefully be:

  • repr
  • force_text
  • saferepr (much slower than the 2 above)
  • pformat (much slower than saferepr)

@codecov
Copy link

codecov bot commented Aug 6, 2018

Codecov Report

Merging #1076 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1076   +/-   ##
=======================================
  Coverage   84.29%   84.29%           
=======================================
  Files          24       24           
  Lines        1318     1318           
  Branches      178      178           
=======================================
  Hits         1111     1111           
  Misses        157      157           
  Partials       50       50
Impacted Files Coverage Δ
debug_toolbar/panels/templates/panel.py 85.93% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6647708...06bafad. Read the comment docs.

@matthiask
Copy link
Member

Makes sense to me. Thanks!

@matthiask matthiask merged commit 06bafad into django-commons:master Sep 11, 2018
matthiask added a commit that referenced this pull request Sep 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants