[Django] #33098: Micro-optimisation for functional.keep_lazy for single argument uses.
Django
-------------------------------------+-------------------------------------
Reporter: Keryn | Owner: Keryn Knight
Knight |
Type: | Status: assigned
Cleanup/optimization |
Component: Template | Version: dev
system |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
`keep_lazy` has an internal decorator function, `wrapper` which may get
called a lot during template rendering, because it's ultimately used by
`conditional_escape` which is in turn used by `render_value_into_context`.
Rendering the standard admin change form for a user via
`client.get(f"/auth/user/1/change/")` 100 times gives me the following
cprofile output:
{{{
11694527 function calls (11041473 primitive calls) in 8.609 seconds
Ordered by: internal time
ncalls tottime percall cumtime percall filename:lineno(function)
30200/300 0.307 0.000 6.703 0.022 defaulttags.py:160(render)
6434 0.242 0.000 0.250 0.000 {built-in method io.open}
174600 0.218 0.000 0.682 0.000 base.py:849(_resolve_lookup)
194000 0.204 0.000 1.120 0.000 base.py:698(resolve)
29200/500 0.175 0.000 6.720 0.013 loader_tags.py:168(render)
30302 0.162 0.000 0.735 0.000 base.py:654(__init__)
34240 0.161 0.000 0.355 0.000 base.py:779(__init__)
15137/5509 0.157 0.000 1.555 0.000 base.py:455(parse)
91639 0.144 0.000 0.542 0.000 functional.py:226(wrapper).
# 1.6%?
...
}}}
with `wrapper` being the important line, and then:
{{{
In [1]: from django.utils.html import escape
In [2]: %timeit escape('<abc>d&g</abc>')
2.2 µs ± 51.3 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
}}}
This is because the current implementation is:
{{{
if any(isinstance(arg, Promise) for arg in itertools.chain(args,
kwargs.values())):
return lazy_func(*args, **kwargs)
return func(*args, **kwargs)
}}}
that is, it's optimised for the general case of N arguments. But Django's
internal usage of `keep_lazy` is nearly unanimously on functions with a
single argument.
It is possible to derive (at decorator time rather than call time) whether
or not the function being wrapped needs to support multiple arguments, via
`inspect.signature` and dispatch to a different wrapper, which offers
somewhat better performance.
A super naive implementation looks like:
{{{
@wraps(func)
def keep_lazy_single_argument_wrapper(arg):
if isinstance(arg, Promise):
return lazy_func(arg)
return func(arg)
@wraps(func)
def keep_lazy_multiple_argument_wrapper(*args, **kwargs):
if any(isinstance(arg, Promise) for arg in itertools.chain(args,
kwargs.values())):
return lazy_func(*args, **kwargs)
return func(*args, **kwargs)
if len(inspect.signature(func).parameters) == 1:
return keep_lazy_single_argument_wrapper
else:
return keep_lazy_multiple_argument_wrapper
}}}
which provides the best difference:
{{{
11327832 function calls (10674778 primitive calls) in 9.062 seconds
Ordered by: internal time
ncalls tottime percall cumtime percall filename:lineno(function)
...
91639 0.059 0.000 0.339 0.000
functional.py:227(keep_lazy_single_argument_wrapper). # 0.6% of time
}}}
and the actual usage time:
{{{
In [2]: %timeit escape('<abc>d&g</abc>')
1.5 µs ± 16.5 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
}}}
Though it comes at the cost of no longer being able to use keyword
arguments:
{{{
In [3]: escape(text=1)
TypeError: keep_lazy_single_argument_wrapper() got an unexpected keyword
argument 'text'
}}}
which can be worked around by doing something (back of the napkin) like:
{{{
func_params = inspect.signature(func).parameters
func_first_param_name = tuple(func_params.keys())[0]
@wraps(func)
def keep_lazy_single_argument_wrapper(*args, **kwargs):
if (args and isinstance(args[0], Promise)) or ('func_first_param_name'
in kwargs and isinstance(kwargs.get(func_first_param_name), Promise)):
return lazy_func(*args, **kwargs)
return func(*args, **kwargs)
...
}}}
which still seems to be better:
{{{
11327896 function calls (10674842 primitive calls) in 9.411 seconds
Ordered by: internal time
ncalls tottime percall cumtime percall filename:lineno(function)
...
91639 0.088 0.000 0.382 0.000
functional.py:230(keep_lazy_single_argument_wrapper). # 0.9%
}}}
and:
{{{
In [2]: %timeit escape('<abc>d&g</abc>')
1.64 µs ± 32.8 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops
each)
In [3]: escape(text=1)
'1'
}}}
**and** correctly works with invalid kwargs (this is as much a note to
myself as anything, my previous attempts did not :)):
{{{
In [4]: escape(test=1)
TypeError: escape() got an unexpected keyword argument 'test'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33098>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Knight
Cleanup/optimization | Status: assigned
Component: Template system | 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
-------------------------------------+-------------------------------------
Comment (by Keryn Knight):
PR is here: https://github.com/django/django/pull/14849
Currently waiting to see if a new revised version works, because it's too
hot here at the moment, and there's a couple of problems with the variant
proposed upthread. Most notably, checking for `'func_first_param_name' in
kwargs` like a fool, but also additionally handling funcs which themselves
declare `*args` or `**kwargs`.
Performance remains roughly the same for single argument functions, unless
they're using the kwarg form, in which case it's something like:
{{{
In [3]: %timeit escape(text='<abc>d&g</abc>')
1.89 µs ± 73.6 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops
each)
}}}
Ain't done any tests for it yet, because I shudder at the thought, knowing
that they'll probably require using mock, and there's definitely little
point doing them if the ticket doesn't have merit :)
--
Ticket URL: <https://code.djangoproject.com/ticket/33098#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Knight
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
OK, looks reasonable — with suitable tests[*], and comments. Thanks Keryn.
[*]: "...being able to use keyword arguments..." — this is documented API,
so we need to not change the behaviour (unless that argument is made
separately...)
--
Ticket URL: <https://code.djangoproject.com/ticket/33098#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Knight
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
Let's go ahead and mark ''Needs Tests'' then, because I sure as heck
haven't added any :)
--
Ticket URL: <https://code.djangoproject.com/ticket/33098#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Knight
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 1 => 0
Comment:
I've added tests, but they're not _good_ ones, really. Hence this remains
on patch needs improvement.
Specifically the tests demonstrate that re-wrapping with a different
specialised wrapper doesn't cause the exceptions to _change_ when called
incorrectly, and covers calling it in a bunch of different ways. **But**
the tests _don't_ demonstrate which wrapper they passed through, or that
they correctly use the `lazy_func`. I'd need some guidance on how to
approach testing those aspects.
Linters are currently preventing it from being marked as successful
(because of course they are), but the tests ''pass'', such as they are.
--
Ticket URL: <https://code.djangoproject.com/ticket/33098#comment:4>