New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed #29343, #28054 -- Implement basic HEAD support for runserver #10676
Conversation
if self.environ['REQUEST_METHOD'] == 'HEAD': | ||
# Consume the iterator for possible side effects | ||
for content in self.result: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maguayo do you have a better suggestions on how to consume an iterator without materializing it? That seems the most elegant way of doing it to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charettes I agree with you. Maybe this is better:
for _ in self.result:
pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the following?
collections.deque(self.result, maxlen=0) # consume iterator quickly.
Some performance numbers:
Python 3.7.0 (default, Sep 15 2018, 19:13:07)
Type 'copyright', 'credits' or 'license' for more information
IPython 6.5.0 -- An enhanced Interactive Python. Type '?' for help.
In [1]: from collections import deque
...: from itertools import repeat
...: n = 10000
...: %timeit -n1000 [__ for __ in repeat(0, n)]
...: %timeit -n1000 for __ in repeat(0, n): pass
...: %timeit -n1000 list(repeat(0, n))
...: %timeit -n1000 deque(repeat(0, n), maxlen=0)
195 µs ± 5.2 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
74.3 µs ± 204 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
53.2 µs ± 4.66 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
43 µs ± 428 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
So even calling list()
seems better than an empty loop, and collections.deque()
is fastest because it is written in C.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to pick the most readable in this case. Some more benchmarks:
For n=10
In [10]: %timeit -n1000 [__ for __ in repeat(0, n)]
778 ns ± 67.2 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
In [11]: %timeit -n1000 for __ in repeat(0, n): pass
345 ns ± 1.85 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
In [12]: %timeit -n1000 list(repeat(0, n))
744 ns ± 36 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
In [13]: %timeit -n1000 deque(repeat(0, n), maxlen=0)
921 ns ± 38.6 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
For n=1
(the most common case?)
In [15]: %timeit -n1000 [__ for __ in repeat(0, n)]
503 ns ± 7.56 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
In [16]: %timeit -n1000 for __ in repeat(0, n): pass
278 ns ± 24.7 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
In [17]: %timeit -n1000 list(repeat(0, n))
699 ns ± 37.7 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
In [18]: %timeit -n1000 deque(repeat(0, n), maxlen=0)
865 ns ± 37.2 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
In this benchmarks the for loop is faster. One should also check how much influence the iterator (repeat
) has, but I think it is not worth the time :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to set up the iterator in each loop executed by %timeit
otherwise it only consumes on the first loop after which it is empty...
I guess it depends on the size of the response being consumed. If you used HEAD
on a response which was reasonably large, then the plain for loop is going to be slower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to set up the iterator in each loop executed by
%timeit
otherwise it only consumes on the first loop after which it is empty...
I am not sure what you mean. I have to check the documentation for %timeit
I guess it depends on the size of the response being consumed. If you used
HEAD
on a response which was reasonably large, then the plain for loop is going to be slower.
What about memory allocation?
In [21]: def large_body(n=100):
...: return repeat(b'spam' * 1024 * 1024 * 5, n)
...:
In [24]: %timeit -n100 [__ for __ in large_body()]
5.8 ms ± 62.6 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
In [25]: %timeit -n100 for _ in large_body(): pass
5.94 ms ± 56.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
In [26]: %timeit -n100 list(large_body())
5.87 ms ± 63.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
In [27]: %timeit -n100 deque(large_body(), maxlen=0)
5.73 ms ± 91.4 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
In this case the elements all share the same storage (the string is the same for each iteration). If you construct a fresh string each time:
In [29]: def large_body(n=100):
...: for _ in range(n):
...: yield b'spam' * 1024 * 1024 * 5
...:
In [42]: %timeit [__ for __ in large_body(50)]
710 ms ± 9.39 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
In [43]: %timeit for __ in large_body(50): pass
277 ms ± 3.18 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
In [44]: %timeit list(large_body(50))
714 ms ± 3.32 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
In [45]: %timeit deque(large_body(50), maxlen=0)
279 ms ± 2.68 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
For smaller but still large responses
In [46]: %timeit [__ for __ in large_body(10)]
54.1 ms ± 768 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
In [47]: %timeit for __ in large_body(10): pass
57 ms ± 865 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
In [48]: %timeit list(large_body(10))
52.6 ms ± 591 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
In [49]: %timeit deque(large_body(10), maxlen=0)
57.5 ms ± 479 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
edit: To conclude, this is nothing to worry about in my opinion. It also also for the development server only. Furthermore, the for loop version expresses the intent most cleary (IMHO) and doesn't allocate additional memory in contrast to all other variants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit: To conclude, this is nothing to worry about in my opinion. It also also for the development server only. Furthermore, the for loop version expresses the intent most cleary (IMHO) and doesn't allocate additional memory in contrast to all other variants.
are they still valid now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's nearly three years on! Goodness... Let's see how things have progressed:
Python 3.9.6 (default, Jun 30 2021, 10:22:16)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.26.0 -- An enhanced Interactive Python. Type '?' for help.
In [1]: from collections import deque
...: from itertools import repeat
In [2]: n = 100
...: %timeit -n1000 for __ in repeat(0, n): pass
...: %timeit -n1000 list(repeat(0, n))
...: %timeit -n1000 deque(repeat(0, n), maxlen=0)
1.06 µs ± 126 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
811 ns ± 97.7 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
971 ns ± 63.4 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
In [3]: n = 10000
...: %timeit -n1000 for __ in repeat(0, n): pass
...: %timeit -n1000 list(repeat(0, n))
...: %timeit -n1000 deque(repeat(0, n), maxlen=0)
80.9 µs ± 642 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
34.1 µs ± 352 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
24.2 µs ± 689 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)
Clearly in this case deque(..., maxlen=0)
still trumps for __ in ...: pass
.
We are also already using this technique:
django/django/http/multipartparser.py
Line 597 in 551c997
collections.deque(iterator, maxlen=0) # consume iterator quickly. |
My only thought for improving this would be to do from collections import deque
to avoid attribute lookup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking 👍
be69360
to
83ec52a
Compare
Closing due to inactivity. |
Tickets: #29343, #28054.
This PR implements the change I suggested in the first ticket.
I could not find HTTP related tests for
runserver
, so I would need some help there. Should this be covered by tests? It seems, this is more patchingwsgiref
.To the best of my knowledge,
sendfile
is not implemented for the builtin server. Should the check be removed? It is copied fromwsgiref
.