-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Reuse expiration_interval
as select()
timeout
#401
Conversation
Lint does not like the additional "cognitive complexity". However, the new code does only make sense exactly there. Not sure whether to ignore or how to resolve. One idea: The |
Codecov Report
@@ Coverage Diff @@
## master #401 +/- ##
==========================================
- Coverage 80.50% 80.15% -0.36%
==========================================
Files 28 28
Lines 4390 4393 +3
==========================================
- Hits 3534 3521 -13
- Misses 856 872 +16 |
@webknjaz About the
The below solves the cognitive complexity CI failure, while of course it doesn't solve it in real as the condition has just moved into select_timeout = min(expiration_interval, 0.05 if IS_WINDOWS else expiration_interval) Furthermore it is too long (>79 characters) to satisfy CI. Hence I left the previous condition syntax, which looks more clear to me, also indicating more explicitly that only on Windows we still need a capped I can btw squash the commits the next days. Currently I'm on the road, having GitHub desktop only here, which does not allow to amend to pushed commits π. |
I was trying to look out for maintainability+readability. The reason is that having random numbers in the middle of a Python
I was talking about the current default
Yeah, and it still hides that half a second timeout under *NIX.
I don't really like how the current change hides the substitution of the default from
That's alright, it's not urgent, desktop and mobile apps are pretty limited. |
Do you think it makes sense to leave a comment why the previously used
It tells you why we need a timeout at all (maybe not clear enough?), which is (on *nix) only to ensure that the EDIT: Different topic, but actually the expiry could be done in a dedicated loop with EDIT2:
Since import ...
...
_WINDOWS_SELECT_TIMEOUT = 0.05
...
select_timeout = min(expiration_interval, _WINDOWS_SELECT_TIMEOUT) but to not force people scrolling up and down to get the whole reason behind this, we'd need to explain why Windows requires this capped timeout once at the variable declaration and once in the function (like it is done already). The more I think about it, the more I think it makes sense to make it a user-choosable input, naming it like |
@MichaIng that sounds reasonable. I just want to clarify that docstrings have a different purpose than code comments. Docstrings explain how things are but code comments give more insight into why/how the implementation decisions were made. P.S. @mar10 do you want to test this with your webdav project? |
@MichaIng oh, and do you think this change could be tested somehow automatically in the CI? |
Makes sense. I'm obviously no Python coder, no professionally trained coder at all, hence still learning about such conventions π. I can split off something from the docstring into a code comment (which is hashed lines
Ah yes, that makes sense. I'll tie this into the commit message. As of that, "Good commit messages" test fails:
It does not work on PRs from forks as the repo/owner from where the test is triggered is used, not the one of the originating branch: https://github.com/platisd/bad-commit-message-blocker/blob/master/entrypoint.sh
Is there some convention about a prefix/tag in the subject line, like
Not sure how. There is two things:
For such things a benchmark as part of CI pipeline would be great, AFAIK the one used in the originating PR to detect the issue on Windows is part of the repo already, so could be made a workflow of? We would not be able to test whether this PR reduces idle CPU load, but whether it affects connection handler performance, especially on Windows. |
Ah, I didn't realize. You're doing great! If you need pointers on some materials to read,
Yes to the leading- These two types of content have different target audiences β the docstring may be OTOH the code comments are meant for code contributors and project archeologists Having magic constants in code is one of the cases where the reader would see an
Ah, interesting, I didn't realize. This should probably be reported upstream.
Yeah, in general I expect the commit messages to follow the same standards as In case, you'd like to dive deep into Git, here's a collection of links I posted some As for 50/72 rule, the commit title / subject line is limited to 50 chars but the prose Currently, I'm trying not to be strict about the Git history (although it's important), How about Reuse `expiration_interval` as `select()` timeout
That's what I thought. Plus depending on the load on the CI platform, some VMs may |
This is a continuation of: #352 As discussed in that pull request, on Windows the select() method does not return when a socket is ready. While the reason is still to be found out, to get the benefit of using a usually much higher timeout merged, Windows is not handled differently: The timeout is capped to 0.05 seconds to assure that connections are not delayed more than that. 0.05 seconds are used as an empirically obtained balance between max connection delay and idle system load. Benchmarks show a mean processing time per connection of ~0.03 seconds on Linux and with 0.01 seconds timeout on Windows: #352 While this highly depends on system and hardware, 0.05 seconds max delay should hence usually not significantly increase the mean time/delay per connection, but significantly reduce idle system load by reducing socket loops to 1/5 with 0.01 seconds. Signed-off-by: MichaIng <[email protected]>
I added a code comment + commit message paragraph now to give further details about 0.05s. I hope it is not too long π: There are at least three benchmarks on the original PR with different systems which all show a mean connection processing time of ~0.03 seconds (on Windows when having a 0.01s timeout). Given that 0.05s on Windows means and average 0.025s delay before a connection is processed, this does not significantly add further delay in those tested cases, but reduces idle socket loops, which are the major idle CPU usage factor, to 1/5th (compared to 0.01s). Commit message line length now capped: https://github.com/MichaIng/cheroot/pull/1
The test still fails as it additionally checks the previous commit which does not satisfy the line length π. This is as the |
expiration_interval
as select()
timeout
@MichaIng this is great! As for the comment/commit size β I don't think that it correlates with the usefulness. It's okay to have them big if that's what's necessary to explain all the context. |
Great, many thanks for merging. I'm looking forward for the next release so that we can finally migrate with our project to latest CherryPy with cheroot ππ. |
Yeah, I need to make up my mind regarding a feature vs a bugfix release number. After that, it should be ready soon. |
I think I'll go for v8.6.0 because this patch has a behavior change / feature to influence the underlying timeout from the exposed settings. |
Makes sense. This is something to not have merged automatically via dependabot-like CI/CD pipeline without having a look at the changelog, at least when a project generally aims to be used on Windows as well. If there are any cases of negative performance import, it's good when it can be associated to this change easily so we can think about exposing it as variable or so. |
`cheroot` v8.6.0 has been released, used in CherryPy v9.0 and above, which has the high idle CPU load resolved: cherrypy/cheroot#401 Since this was the reason for using a CherryPy<9.0 version dependency, this is hereby removed. Signed-off-by: MichaIng <[email protected]>
I re-run my older tests on new Hardware. IIRC, 8.3.1 was the last originally 'problematic' Cheroot release? Windows 11, Ryzen 5 @ 3.9 GHz Runtime 30 secs, 10 parallel sessions This is what I see on Windows:
My script-runner and WebDAV server my not be perfect for absolute benchmarks, but the comparison is still interesting. I can re-run on Mac later. |
Oh boy, still a major requests per second difference on Windows. The benchmarks in #352 done by you and others showed an average performance of 0.02 - 0.035 seconds per request (now yours is ~0.013 with v8.5.2). Given a random request is coming in when in average 0.025 seconds timeout are left (half of the now hardcoded maximum of 0.05 on Windows), I'd expected this to not change significantly. But of course it depends on the performance of the underlying system, and how the benchmark is done. E.g. at higher concurrency (instead of doing the requests one after another), the timeout looses weight as the processing of the accumulated incoming connections since the last loop gains weight. I think the major difference which is relevant is that the maximum possible delay for a random incoming connection on Windows increased from 0.01 to 0.05 seconds, and the question is whether this is noticeable by users or not. I did some fresh idle CPU usage comparison of our project on a Linux VM. The host is a notebook, some years old, but full power given to the VM, and the idle CPU usage dropped from ~2-3 seconds per minute (on a single core) with v8.5.2 to ~0.3 seconds per minute with v8.6.0, with the default 0.5 seconds timeout there => up to 2 loops per second. On a Raspberry Pi 1/Zero this is much more significant, of course, hence a huge benefit for using cheroot/CherryPy on embedded low-performance systems. Would be interesting to see whether on Windows with now only up to 20 instead of 100 loops per second, there is a reduced idle CPU usage measurable as well, which balances the additional connection delay sufficiently. Otherwise, of course, we can use again |
My server and also the test tool are written in Python, so multithreading performance is not-so-good. I would not refer to the numbers for absolute request timings. I was also running server and tool in two shells on the same box. Here's my results now with macOS tests: WsgiDAV 4.0.0-a2 Windows 10, Ryzen 5 @ 3.9 GHz
macOS 12, i5 @ 2.9 GHz
|
On Windows, could you raise concurrency, e.g. 20 parallel sessions, to see whether this leads to closer results as expected? I'll also spin up Python again on my Windows machine to repeat tests with https://github.com/cherrypy/cheroot/blob/master/cheroot/test/test_wsgi.py and your stressor with timeout values between 0.01 and 0.05. |
Windows 10, Ryzen 5 @ 3.9 GHzSame scenario, different no. of parallel users. Best of 3
|
I did not follow the thread, so sorry if this has already been discussed: |
Oh wow, so 20 parallel users vs 10 parallel users completely eliminates the difference between
This hardcoded timeout is used on Windows only, so also
In theory more magic could be added, but given that it is all a workaround for a bug or at least undocumented behaviour, I personally don't think that it is good invested development time, and the more complicated it gets the more the risk is the we have unexpected downsides on other ends. Also, given your benchmarks, I don't think that there is much potential for significant real world performance improvements, rather than idle CPU load improvements on a typical Windows system/server. |
Agreed. I just tested copying a few hundred files using Windows 11 File Explorer mapped to a WebDAV folder and got similar performance for cheroot 8.5.2 and 8.6.0. |
Fix PR #401 description in changelog
I also ran some tests again with #352 (comment) and ranged concurrency/
And with
matching very closely the result with
The differences are all within the error range between test runs, and I did a lot of repeating whenever changing a value. I then also raised the overall number of connections with So while there are still open questions, and it would be better to do a benchmark with server and client(s) running on different systems, I think that |
`cheroot` v8.6.0 has been released, used in CherryPy v9.0 and above, which has the high idle CPU load resolved: cherrypy/cheroot#401 Since this was the reason for using a CherryPy<9.0 version dependency, this is hereby removed. Signed-off-by: MichaIng <[email protected]>
β What kind of change does this PR introduce?
π What is the related issue number (starting with
#
)#378
cherrypy/cherrypy#1908
β What is the current behavior? (You can also link to an open issue here)
The connection handler loop runs once every ~0.01 sections, leading to a significant idle system load on small hardware.
β What is the new behavior (if this is a feature change)?
The connection handler loop runs once every ~
expiration_interval
, which is required to assure that expired connections are closed in time, which is done in the same loop. On Windows, the maximum timeout is capped to 0.05 seconds to make this the maximum additional delay whenselect()
does not return once a socket is ready.π Other information:
This is a continuation of: #352
π Checklist:
and description in grammatically correct, complete sentences
This change isβ