Opened 6 years ago

Last modified 6 years ago

#14365 new New feature

Make template-rendering signals available also in DEBUG mode

Reported by: Carl Meyer Owned by: Carl Meyer
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Django's test setup monkeypatches the template renderer to send a signal every time a template is rendered. The test client handles this signal to provide template-rendered debugging information.

It would be useful to have access to this signal in DEBUG mode, not only in testing, so that the test Client can be used from the python shell without losing the template info, and so external tools like django-debug-toolbar can make use of it without having to monkeypatch.

The original reason for the current behavior was performance; signals used to be quite slow. This is no longer the case (and it should be possible to make the performance impact in production nothing more than a single DEBUG check at startup/init time). Nonetheless, any patch here should come with performance impact data.

Attachments (2)

14365_r13962.diff (6.1 KB) - added by Carl Meyer 6 years ago.
14365_djangobench.txt (2.1 KB) - added by Carl Meyer 6 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 6 years ago by Carl Meyer

Triage Stage: UnreviewedAccepted
Version: 1.2SVN

Marking Accepted based on IRC approval-in-concept (depending on performance impact) from Russell and Jacob.

comment:2 Changed 6 years ago by Carl Meyer

Has patch: set
Owner: changed from nobody to Carl Meyer

Here's my initial go at a patch. It bases the sending of the signal on the value of TEMPLATE_DEBUG, and setup_test_environment forces the sending of the signal regardless.

As you can see, this patch does a boolean if check on every template render. This is the simplest, most straightforward code, so I decided to try it first and check the performance impact. I added a template_render_simple benchmark to djangobench (available at http://github.com/carljm/djangobench/tree/template_render_simple) to focus on the render call itself and eliminate as much noise as possible. Results also attached: djangobench certainly isn't detecting any noticeable slowdown; half the time it seems to think it's faster, which suggests to me that any impact is so small as to be completely insignificant.

If someone can detect a statistically significant slowdown from this change, or suggest any alternative benchmarking methods that might detect one, I have ideas for how the boolean check could be done just once at import time. But all of these ideas would involve less readable and maintainable code, so I'm going to vote for the simple approach in the absence of any evidence of a performance issue.

Changed 6 years ago by Carl Meyer

Attachment: 14365_r13962.diff added

Changed 6 years ago by Carl Meyer

Attachment: 14365_djangobench.txt added

comment:3 Changed 6 years ago by Carl Meyer

comment:4 Changed 6 years ago by Russell Keith-Magee

Needs documentation: set

Patch looks good, except for the absence of docs. We're adding a new public signal, and an API that would allow people to subclass (or monkeypatch) Template to turn on that signal.

Actually, that makes me wonder if Template should maybe take a 'enable signals' argument so you can turn on signals on a per-instance basis, rather than needing to subclass or monkeypatch...

comment:5 Changed 6 years ago by Carl Meyer

Patch needs improvement: set

Actually, on further review, this patch needs more work. I used a class attribute on Template to avoid a check every time a template is instantiated, but introducing an import-time settings dependency is not OK.

Given that there's already a TEMPLATE_DEBUG check in Template.init, I can probably piggyback on that with almost zero perf impact (I'll benchmark, of course), and having the API be via an arg to Template.init (defaulting to TEMPLATE_DEBUG) is nicer than a class attribute (which is basically a global). Just need to figure out how to allow the test system to force it to True...

comment:6 Changed 6 years ago by Julien Phalip

Severity: Normal
Type: New feature

comment:7 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:8 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

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