-
Notifications
You must be signed in to change notification settings - Fork 78
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
async API #569
async API #569
Conversation
Preliminary results on:
1.4x faster on a SSH target (juno r0):
2.2x faster on a local target:
|
1b2cc97
to
fcd8d4b
Compare
I checked all instances of threading.Lock in devlib and it should be safe, as none is in the path used when running a background command or any other coroutine. If a function taking a threading.Lock was being called by 2 coroutines that end up running concurrently, the 2nd coroutine would wait for the lock, thereby blocking the main thread and deadlocking. |
I also tried various combinations of thread pools, including running a blocking execute() in a different thread (so with separate SSH connections) and I did not get any real timing variations. What I did get is some errors probably because I was maintaining too many connections on the SSH server, so the current approach with one connection and many concurrent SSH channels seems to be the best. I could not saturate the server with open channels (maybe there is no limit). If that was to happen, we could use an asyncio.Semaphore to limit the number of bg commands in flight. |
fcd8d4b
to
798a0a3
Compare
There are a few things to cleanup (docstring etc) so if you are happy with the API as it stands I will proceed with:
EDIT: cleanup done. Beyond possible high level doc and converting more bits, the change is somewhat ready. |
c42be20
to
7a7fead
Compare
7a7fead
to
d11b2de
Compare
While working on a bug fix for another problem, it came to my attention that SSH servers accept a fixed number of "sessions" per connection. This "sessions" seem to map to paramiko's channels. This means that for a given connection, we might be limited to e.g. 10 concurrent channels (default of OpenSSH). This is configurable with I'll probably need to handle that in that PR to limit the number of opened channels. There does not seem to be a standard way of getting the max number of channels, but we can probably handle the exception as a hint, or attempt to read OpenSSH config and parse it to find the |
6f03604
to
1ebc1a3
Compare
@setrofim @marcbonnici I've updated the PR with an automatic detection of number of allowed background commands, along with some automatic limiting in the async API to use at most half of the available channels. Next step:
|
f89adf9
to
8679e42
Compare
Updated the PR with checks to make sure that no file access conflicts when executing concurrent asyncio tasks. An artificial example:
In the PR, Note: this depends on Python >= 3.7 for |
9c4c402
to
6844720
Compare
f92e27a
to
659a4ff
Compare
bb68b69
to
2b8bd69
Compare
FWIW we have been using this PR in Lisa for a month now (Lisa repo vendorizes devlib as a git subtree) and so far I haven't observed or heard of any issue related to that |
Implement __aenter__ and __aexit__ on nullcontext so it can be used as an asynchronous context manager.
The conncetion returned by Target.get_connection() does not have its .busybox attribute initialized. This is expected for the first connection, but connections created for new threads should have busybox set.
Remove all the tls_property from the state, as they will be recreated automatically.
Remove dependencies that are ruled out due to the current Python minimal version requirement.
Require Python >= 3.7 in order to have access to a fully fledged asyncio module.
1bace01
to
4a99e78
Compare
@marcbonnici PR updated with:
EDIT: Forgot to add the last bullet entry |
4a99e78
to
7b3d726
Compare
Home for async-related utilities.
Add async variants of Target methods.
Allow the user to set a maximum number of conrruent connections used to dispatch non-blocking commands when using the async API.
Make use of the new async API to speedup other parts of devlib.
PR re-updated with a lazy async generator blocking shim. This preserves the laziness of the async gen, made possible by |
7b3d726
to
843e954
Compare
use_governor() was trying to set concurrently both per-cpu and global tunables for each governor, which lead to a write conflict. Split the work into the per-governor global tunables and the per-cpu tunables, and do all that in concurrently. Each task is therefore responsible of a distinct set of files and all is well. Also remove @memoized on async functions. It will be reintroduced in a later commit when there is a safe alternative for async functions.
Add a memoize_method decorator that works for async methods. It will not leak memory since the memoization cache is held in the instance __dict__, and it does not rely on hacks to hash unhashable data.
Add an async API to devlib to take advantage of concurrency more easily, both at a coarse and fine grain.
Note: This currently requires python 3.7
Based on #568