Skip to content
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

Enable limited use of asyncio with executors #971

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jul 18, 2022

Resolves #962
Alternative to #963

This enables limited use of asyncio primitives with the rclpy executor. The most important parts are the changes to how Future instances handle __await__ and how Task instances get scheduled. They allow Task to tell when an asyncio future is awaited, and they allow Task to pause itself until the asyncio future completes. A consequence of this is Task now requires an executor.

There are limitations coming from asyncio not being thread safe.

  • There must be an asyncio event loop running
  • A single threaded executor must be used, and that executor must spin periodically in the same thread as asyncio's event loop

Changes

  • rclpy Future.__await__ yields itself instead of None - this allows Task to know if an rclpy Future is being awaited
  • Add Executor.call_soon() - This is like create_task(), but if the callback is already a task then it won't create one. The naming is taking from asyncio
  • Executor._tasks is cleared every time tasks are run. Task instances schedule themselves when they become unblocked.
  • If Task detects it's waiting on an asyncio future, then it will make the asyncio future schedule the Task when it's done
  • Some tests had asyncio.sleep(0) in them already, but didn't create an asycnio event loop. Those uses have been removed.
  • I removed Task._lock because it should never run in parallel now that it schedules itself.
  • I removed Task.executing() because it would have needed the Task._lock, and wasn't used anywhere else.

@sloretz sloretz added the enhancement New feature or request label Jul 18, 2022
@sloretz sloretz self-assigned this Jul 18, 2022
@sloretz sloretz force-pushed the sloretz__rclpy__asyncio_interop branch from 3867151 to e97fdea Compare July 22, 2022 20:37
@sloretz
Copy link
Contributor Author

sloretz commented Jul 22, 2022

This PR also seems to be a small performance improvement. Using the benchmarks from #973 (1527a9e) and running each test 5 times on each branch there's improvement in every single test. The average improvement is 1.75%.

Benchmark Timer Timer coro Guard Guard coro pub/sub client/server action
master 162.1116 162.1116 159.8493 158.453 242.1545 528.9525 680.5634
  166.2661 166.513 166.1291 167.0961 237.8923 524.6592 658.3939
  164.1507 164.2656 164.2676 164.256 234.6498 527.6705 679.2866
  165.3802 164.6015 162.6969 161.9369 237.4209 516.9322 683.0678
  165.8009 165.1686 164.9115 165.091 237.8524 511.6613 691.9759
Average 164.7419 164.53206 163.57088 163.3666 237.99398 521.97514 678.65752
with 971 159.8871 159.4097 160.1809 163.3935 228.803 517.9435 655.3708
  163.6177 163.1115 165.7945 165.0023 224.5216 523.299 691.9127
  162.6403 158.5519 159.661 158.085 235.0898 518.9754 666.6267
  160.8633 161.9503 158.0991 163.3056 234.2222 520.8381 667.9764
  163.3952 163.5464 156.8233 156.1533 224.0261 521.4153 666.8869
Average 162.08072 161.31396 160.11176 161.18794 229.33254 520.49426 669.7547
               
Percent diff -1.615363183 -1.955910599 -2.114752944 -1.33360185 -3.639352558 -0.283706998 -1.311828093
               
Average -1.750645175            

@sloretz
Copy link
Contributor Author

sloretz commented Jul 22, 2022

CI (build: --packages-above-and-dependencies rclpy test: --packages-above rclpy)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@Achllle
Copy link
Contributor

Achllle commented Apr 11, 2023

@sloretz Do you still see this approach as the best way forward? It would be helpful to know what the best approach is to marry asyncio and rclpy as there doesn't seem to be one currently, and this PR seems to be in limbo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime error when a long async callback is triggered
2 participants