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 Tim McCurrach)

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 Tim McCurrach, 4 years ago

Owner: changed from nobody to Tim McCurrach

comment:2 by Tim McCurrach, 4 years ago

Description: modified (diff)

comment:3 by Tim McCurrach, 4 years ago

Version: 3.1dev

comment:4 by Carlton Gibson, 4 years ago

Component: UncategorizedTemplate system
Triage Stage: UnreviewedAccepted

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.

comment:5 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In cda81b79:

Refs #32568 -- Optimized escape() by using SafeString instead of mark_safe().

comment:6 by Mariusz Felisiak, 9 months ago

Owner: Tim McCurrach removed
Status: assignednew

comment:7 by Nina Menezes, 3 months ago

Owner: set to Nina Menezes
Status: newassigned

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?

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