[Django] #32568: Prefer SafeString to mark_safe where possible
Django
------------------------------------------------+--------------------------
Reporter: Tim McCurrach | Owner: nobody
Type: Cleanup/optimization | Status: assigned
Component: Uncategorized | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+--------------------------
`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 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/32568>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Tim McCurrach | Owner: Tim
Type: | McCurrach
Cleanup/optimization | Status: assigned
Component: Uncategorized | Version: 3.1
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim McCurrach):
* owner: nobody => Tim McCurrach
--
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:1>
Django
Reporter: Tim McCurrach | Owner: Tim
Type: | McCurrach
Component: Uncategorized | Version: 3.1
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Tim McCurrach:
Old description:
> `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 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?
New description:
`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?
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:2>
Django
Reporter: Tim McCurrach | Owner: Tim
Type: | McCurrach
Component: Uncategorized | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim McCurrach):
* version: 3.1 => dev
--
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:3>
Django
Reporter: Tim McCurrach | Owner: Tim
Type: | McCurrach
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* component: Uncategorized => Template system
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:4>
Django
Reporter: Tim McCurrach | Owner: Tim
Type: | McCurrach
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"cda81b79f212e0666782393c52ad19c2790c9446" cda81b79]:
{{{
#!CommitTicketReference repository=""
revision="cda81b79f212e0666782393c52ad19c2790c9446"
Refs #32568 -- Optimized escape() by using SafeString instead of
mark_safe().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:5>
Django
Reporter: Tim McCurrach | Owner: (none)
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Tim McCurrach => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:6>
Django
Reporter: Tim McCurrach | Owner: Nina
Type: | Menezes
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nina Menezes):
* owner: (none) => Nina Menezes
* status: new => assigned
Comment:
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`?
--
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:7>
Django
Reporter: Tim McCurrach | Owner: (none)
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nina Menezes):
* owner: Nina Menezes => (none)
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:8>
Django
Reporter: Tim McCurrach | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Template system | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Pravin):
I will work on this
--
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:9>
Django
Reporter: Tim McCurrach | Owner: Pravin
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Pravin):
* owner: (none) => Pravin
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:10>
Django
Reporter: Tim McCurrach | Owner: Pravin
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Pravin):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:11>
Django
Reporter: Tim McCurrach | Owner: Pravin
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Changes (by Jacob Walls):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32568#comment:12>