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

Add future runtime to libtock-rs #103

Merged
merged 17 commits into from
Nov 22, 2019
Merged

Conversation

torfmaster
Copy link
Collaborator

Summary

This PR adds a MVP for async/await support. The benefit hypothesis is that the async/await approach is easier to use and better fits the requirements for concurrency than the current callback approach.

Implementation

The core of the implementation is the block_on executor. In order to use asyncand await we moreover import async-support a patched version of the core crate which adds adds support for compilation of async, await code for #![no_std]. This method is inspired by drone os.

Examples

An examples how to use async/await can be found in the example async.rs.

Out of Scope

Design Decision and Migration

We believe that futures provide a more convenient and accurate way of handling concurrency in libtock-rs. One goal would be to verify and decide this and migrate the whole library to futures, step by step. One step would be to document this decision in the design document.

Multiple usages of the same driver

The tock kernel only supports only callback per driver per app. However, as in the case of the callback interface the library can register multiple callbacks per driver which leads to unexpected behavior. One goal would be to manage this e.g. by providing a Drivers singleton which either manages multiple callbacks or forbids multiple callbacks.
One candidate to test this would be the timer driver as it appears to be a valid use case to have different loops performing work in a different frequency.

Ergonomy

The main function is and will probably stay synchronous. One goal would be to use procedural macros to generate the boilerplate code for a async main.

async-support/src/lib.rs Show resolved Hide resolved
@jrvanwhy
Copy link
Collaborator

I've been thinking about futures implementations for a while, and have some concerns about size (both code size and RAM usage).

What is the size impact of this change on the examples?

@torfmaster
Copy link
Collaborator Author

torfmaster commented Nov 13, 2019

I've been thinking about futures implementations for a while, and have some concerns about size (both code size and RAM usage).

What is the size impact of this change on the examples?

I haven't checked the size but issues will probably only occur with complex code which we don't have at the moment in the examples. As far as I understand, potential bloat comes from code generated by the compiler when converting complex chains of awaits into state machines. As a solution I would propose to

  • implement better code than the compiler would do if possible
  • use blocking code or libtock-c

If size or memory becomes an issue.

@jrvanwhy
Copy link
Collaborator

jrvanwhy commented Nov 13, 2019

I'm more concerned about:

  • The size of the VTables used by Waker/the executor.
  • The dynamic dispatch causing false dependencies that prevent LTO from trimming out unnecessary code (particularly futures).
  • The overhead of the futures Waker mechanism.

EDIT: I'm more concerned about that than the overhead of rustc's async code generation.

@rajivr
Copy link
Contributor

rajivr commented Nov 14, 2019

The benefit hypothesis is that the async/await approach is easier to use and better fits the requirements for concurrency than the current callback approach.

While we still don't have proof for this yet, I agree with the view that having an async/await friendly Tock user-space would be really awesome.

Another open question for me is if might have to make changes to current syscall infrastructure to make it more async/await friendly. If so, how can we do that in a backward compatible way.

I also agree with @jrvanwhy concerns.

Unfortunately I won't be able to get back to exploring async/await till early next year. I am happy to see progress happening here.

Based on my previous exploration with async/await, I was wondering how select and join semantics would work with Drone OS style futures?

I understand why modifications to libcore is necessary and I am glad you've take care of that part! :-)

@Woyten
Copy link
Contributor

Woyten commented Nov 14, 2019

@jrvanwhy The size of the RawWakerVTable is predictable: it is four references. I just rearranged the code a bit, s.t. the Vtable is static and never cloned.

Regarding the Wakers: They are never called, actually. As the implementation of the future is statically known (unless you Box them but why should you?), the compiler will get rid of them completely.

Where do you see dynamic dispatch causing false dependencies that prevent LTO from trimming out unnecessary code? Is it just a bad feeling that this could possibly happen? I do not think we need to be that pessimistic and trust the toolchain until there is a clear observable negative effect.

src/syscalls.rs Show resolved Hide resolved
async-support/src/lib.rs Show resolved Hide resolved
examples/simple_ble.rs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
 - alloc can be used without feature since Rust 1.36
 - remove break from example
CHANGELOG.md Outdated Show resolved Hide resolved
async-support/src/lib.rs Outdated Show resolved Hide resolved
async-support/src/lib.rs Outdated Show resolved Hide resolved
async-support/src/lib.rs Outdated Show resolved Hide resolved
async-support/src/lib.rs Outdated Show resolved Hide resolved
@Woyten
Copy link
Contributor

Woyten commented Nov 18, 2019

All points have been discussed and most of them resulted in action Items. If there are no new comments, this PR will be merged very soon.

@gendx
Copy link
Contributor

gendx commented Nov 18, 2019

Regarding performance concerns, I recently became aware of cargo-call-stack. It seems like a nice way of visualizing the call graph and stack usage of a Rust program. I don't know how compatible it would be with libtock-rs, but it could make sense to try it on some of the examples and show a before/after to measure some of the impact of futures.

If it doesn't work out-of-the-box, please ignore this comment.

@jrvanwhy
Copy link
Collaborator

Regarding performance concerns, I recently became aware of cargo-call-stack. It seems like a nice way of visualizing the call graph and stack usage of a Rust program. I don't know how compatible it would be with libtock-rs, but it could make sense to try it on some of the examples and show a before/after to measure some of the impact of futures.

If it doesn't work out-of-the-box, please ignore this comment.

cargo-call-stack will not work out-of-the-box, but in the long run would be a good tool to have. It would be nice to have a tool that understands how yield can cause stack frames to build on top of each other (i.e. a libtock-rs-specific tool would be nice).

I can probably take another look at this on Wednesday but feel free to submit without me. In the longer term (next couple of months), I intend to build some code size analysis tooling and then compare the impact of futures on the code size of an example app (versus a callback style similar to Tock's kernel).

CHANGELOG.md Show resolved Hide resolved
timer::sleep(Duration::from_ms(100));
for led in led::all() {
led.off();
executor::block_on(async {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems unnecessary for this to build a future if it will loop infinitely. Why not have the loop {} on the outside and call executor::block_in on the sleep calls inside the loop?

I have the same concern about cycle_leds() below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it unnecessary? It makes the code read nicer than having a future for each call of sleep.

I have no strong feelings for this code. It should probably get removed and be replaced by something else. Maybe it should print to the console and restart the application?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The low_level_status_code(1) call above will trigger a print if the LowLevelDebug driver is present; that's probably the best option. I don't think it should restart the application automatically by default, although I could see that being a nice option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a panic! should be visible even if no console is attached if the device is running somewhere "in production" so I like the solution with the blinking leds.

@jrvanwhy
Copy link
Collaborator

Sorry for the delay, I'm still trying to figure out GitHub's review UI.

I think this is fine to merge as-is, although I expect to revisit it in a couple months when I have better code size tooling available.

@torfmaster torfmaster merged commit 089f8c0 into tock:master Nov 22, 2019
@Woyten Woyten deleted the feature/futures branch November 22, 2019 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants