connections: expiration_interval as select timeout #352
Conversation
liamstask
force-pushed
the
connections-select-timeout
branch
from
1162281 to
52cb7ec
Compare
Sure:
Click to expand detailsCheroot 8.5.1Cheroot 8.4.8Cheroot 8.3.1Cheroot 8.2.1Cheroot 8.1.0Cheroot 8.0.0gevent 20.12.1
|
|
@mar10 that log looks like you were testing the released versions but not this PR branch. Is that right? @liamstask rebasing the PR should pull-in the pytest-related fixes should get more jobs green. |
liamstask
force-pushed
the
connections-select-timeout
branch
from
52cb7ec to
2af88bb
Compare
|
rebased |
that's true. and now see errors (PUT timeouts after 0.5 seconds, which is a threshold I set in my test). |
webknjaz
added
bug
|
I removed the timeout check and re-run the test again (30 seconds, 10 parallel users). With this PR I see that most requests seem to take ~0.5 seconds. I searched cheroot for '0.5' and found I then re-run with
For reference, here are the results with cheroot 8.5.1: |
|
Do you think this setting should be a part of the public API? As in, should users be expected to tweak it per their needs similar to the amount threads? |
|
hm, no - this is not expected behavior, as the selector should return immediately once an entry becomes ready to read. will see if i can reproduce and understand what might be responsible. |
|
@liamstask do you want to mark this PR as a Draft or do you want to work on a separate one? Also, is this PR considered a feature or a bugfix? Depending on this, I may consider still publishing it under 8.x (it's still in sync with the master but 9.x is supposed to be Python 3-only). |
|
marking as draft until we understand the nature of this issue - depending on what we find, it may make sense to open another PR. and hm - i think we could probably consider this PR a feature, as the current behavior isn't a problem necessarily. but i do wonder if we should see even lower latency than the |
Hm... I'm curious if that's a side effect of the GIL allowing only one thread at a time. |
|
i am not immediately able to reproduce. i've locally added the following to def test_response_latency(simple_wsgi_server):
import statistics
from timeit import default_timer as timer
session = Session(base_url=simple_wsgi_server['url'])
pooled = requests.adapters.HTTPAdapter(
pool_connections=1, pool_maxsize=1000,
)
session.mount('http://', pooled)
def do_request():
start = timer()
resp = session.get('info')
resp.raise_for_status()
return (timer() - start)
with ThreadPoolExecutor(max_workers=10) as pool:
tasks = [
pool.submit(do_request)
for _ in range(1000)
]
durations = [task.result() for task in tasks]
print("done.")
print("min: {}".format(min(durations)))
print("mean: {}".format(statistics.mean(durations)))
print("max: {}".format(max(durations)))
print("stdev: {}".format(statistics.stdev(durations)))
print("variance: {}".format(statistics.variance(durations)))
assert False # dump stdout to consoleand varied These all look fairly consistent, within a margin of jitter, regardless of the timeout, as would be expected. This is on my local machine - macOS, python3 - I wonder if there could be some unexpected platform-specific Windows behavior in the selector module? @mar10 are you able to run your test on any other platforms to get another data point? |
|
@mar10 if it's not straightforward to run your previous tests on another platform, it would still be useful to run the quick test I posted above on your platform, if possible, to help understand where this divergence might be coming from. @webknjaz if you have a moment to run the test i posted above, or recommend another strategy, that would be helpful too. thanks! |
|
Here's what I get on my Gentoo Linux laptop (P1 Gen2) under Python 3.9: |
|
On an older macbook pro i don't see a performance drop like on windows cheroot 8.5.1cheroot 8.5.2.dev8+g2af88bb8 |
|
@mar10 thank you. 2 other thoughts/requests to test on Windows, if/when you have time:
|
|
Sorry for being so unresponsive, I will try to address the this week... |
|
As of #378 and cherrypy/cherrypy#1908, hopefully we can finish this. Indeed So it would be good to know whether the issue that |
I ran a test tool I wrote against a WebDAV server that uses Cheroot (among others) as WSGI server. |
|
Not sure how to use Unmodified cheroot 8.5.2cheroot 8.5.2 with
|
|
Maybe this is related? Probably someone has an idea how to narrow it down to the naked selector's |
|
I noticed that the CPU utilisation on a raspberry pi 1st generation drops from ~20% to ~10% with cheroot 8.5.2 (or maybe a little earlier), but only when settings 'server.thread_pool' : 1 and/or 'engine.autoreload.on' : False are set before quickstart. This effectively removes the original problem / bug I lodged in #1908 and taken over in #378. So something else must have changed in the meantime..... |
|
It makes sense that the load is reduced when the server threads are reduced, but also 10% idle load is much too high and limiting the threads has negative impact on concurrent connection performance likely. With this commit is should be possible to reduce idle load to 1/5, i.e. something around 5% in your case, without affecting concurrent connection performance. Autoreload is a CherryPy feature, not a cheroot one, and it is disabled by default, isn't it? However, it is another (unrelated) layer of idle load of course, when CherryPy regularly checks for file changes. So while your settings changes are workarounds, it doesn't mean that cheroot performs well or that this PR looses its reason. |
|
"It makes sense that the load it reduced when the server threads are reduced" - well, previously that never made any difference, so something else has happened in the meantime which practically removes the problem. Nevertheless, agreed that this PR is still worthwhile having. |
|
Would it be an option to add a workaround for Windows, to either set a lower I currently do not find the time to nail it down to |
|
I don't use Windows so I can't comment on the suggested workaround. |
|
Basically I mean to leave things for Windows as they are currently, until we find a better solution, but apply this PR/change to all other systems, where it clearly enhances things. Observing idle CPU load with and without this PR should help to build an opinion whether it's worth it on non-Windows systems, and request performance on Windows and non-Windows systems are well covered by the tests above. I'm just not sure how to reliably check whether it's a Windows platform or not in Python? I try to push this topic since the high CPU idle load on small SBCs is currently really a deal breaker for one of our projects, where we're forced to use CherryPy < 9.0 because of this, which has other downsides, obviously. |
|
Understood and makes sense - and I don't use Windows, so fine for me. |
|
To go on, I opened a new PR: #401 |
FWIW since that has mentions of |
|
This PR has been superseded by #401, which was merged now. @mar10 would be great if you could rerun your benchmarks on Windows to verify that the timeout of now Not sure whether the idle CPU usage can be reasonably compared on common (relatively powerful) Windows systems, but on an embedded system/SBC this should be visible, as tested on cherrypy/cherrypy#1908 and #378. This PR can hence be closed. Further discussion, when required, make sense in the merged PR 🙂. |
|
@webknjaz
As far as I understand, the
You mean, instead of "waiting" for a new connection via self._selector.register(
conn.socket.fileno(), selectors.EVENT_READ, data=conn,
) |
You got it. |
❓ What kind of change does this PR introduce?
📋 What is the related issue number (starting with
#)#311 made a change to process connections as they become active, rather than polling. One of the motivations was to ensure that the latency with which connections are processed is not tied to the select timeout, as it was originally - see #305
I originally had this change in #311 and intended to submit with it, but must have somehow dropped it during editing/rebasing.
Also includes a minor change to avoid an unnecessary call to
time.time()when handling expirations.❓ What is the current behavior? (You can also link to an open issue here)
❓ What is the new behavior (if this is a feature change)?
📋 Other information:
📋 Checklist:
and description in grammatically correct, complete sentences
This change is