Opened 4 years ago
Last modified 3 months ago
#32568 assigned Cleanup/optimization
Prefer SafeString to mark_safe where possible
Reported by: | Tim McCurrach | Owned by: | Nina Menezes |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
mark_safe
takes roughly twice as long as simply creating a SafeString
- using pyperf:
mark_safe: Mean +- std dev: 296 ns +- 12 ns SafeString: Mean +- std dev: 158 ns +- 7 ns
There are many places in the django codebase where we know the thing we are marking as safe to be a normal (not marked as safe) string. In such cases it makes sense to use SafeString
instead of mark_safe
.
To play devils advocate, you could definitely argue that this is an unnecessary micro-optimisation. Following a brief search for mark_safe
, there are some situations where we have something like mark_safe(X)
and where evaluating X
will take sufficiently long that any savings made marking the string as safe would be rendered insignificant.
Having said that, there are other places where we end up calling mark_safe
a very large number of times. In such situations a small saving in time will add up to a larger saving. There are also places where we even have mark_safe("some string literal")
.
Furthermore, since this change is literally just replacing one word with an equally clear word, it would have no effect on complexity or readability, and so for a slight performance boost, why not?
Change History (7)
comment:1 by , 4 years ago
Owner: | changed from | to
---|
comment:2 by , 4 years ago
Description: | modified (diff) |
---|
comment:3 by , 4 years ago
Version: | 3.1 → dev |
---|
comment:4 by , 4 years ago
Component: | Uncategorized → Template system |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:6 by , 9 months ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:7 by , 3 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
I'll pick this up as it looks like no-one else is looking at it.
In addition to changing mark_safe
in the code, should instances in the docs change where appropriate e.g. recommend use of SafeString
over mark_safe
?
Hey Tim — I'm going to provisionally accept this, since you assigned it to yourself, and I assume there's a PR on the way?
It seems reasonable enough, but I'd like to have a look at it before saying Yes absolutely.
Thanks.