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

Discussion: Async I/O #430

Closed
greshake opened this issue Aug 21, 2019 · 23 comments
Closed

Discussion: Async I/O #430

greshake opened this issue Aug 21, 2019 · 23 comments

Comments

@greshake
Copy link
Owner

When I first designed i3status-rust, async I/O was still a long way from being usable. Thus my design involves a custom task scheduler to combine all the block computations in a single thread, but I/O still either blocks the main thread or has to be deferred to a separate thread with Mutexes and callbacks.. This is of course not helpful to reach the goal of maximum efficiency and minimal overhead.

In the years since, Rust has made great progress towards async I/O. Futures are stabilized, Tokio has reached 0.2 and the general ecosystem has massively improved. We can now borrow across async tasks too using the Pin trait, which has also been a blocker for me.
We could conceivably reimplement the core system to use futures, but I had built a prototype and it was ergonomic hell, and I don't want to put anyone who wants to contribute through that process. So, I believe we should wait for async/await syntax, whcih has also made great progress (https://areweasyncyet.rs/). We could build a proof-of-concept in the current nightly, but there are still some open issues that prevent me from going for it right now.

Would you guys agree that we should wait for the following features/ or do you think we can or should work around these issues right now?

  • Can't have async fn in traits like the block trait... There is a macro workaround though (https://github.com/dtolnay/async-trait)
  • Can't output streams from async fns without generators (which are still at least 1-2 years away from stable I'd say...). We would benefit from async fn streams ergonomically as every block essentially just outputs a stream of UI updates and receives an async stream of UI input. Alternatively, we could pass a reference to a stream to every block and have them put data in them manually.
@SWW13
Copy link
Contributor

SWW13 commented Aug 21, 2019

I'd definitely wait for stable async/await for a release, but working on a PoC in nightly seems like a good idea.

The macro workaround looks fine to me as it's easy to remove it when the feature has landed.
If your estimation for streams are right then I wouldn't wait that long and start with the reference workaround.


If we do a full rewrite of the core system may I suggest some general improvements:

  • I'd love to have generic format option for all blocks, which could be implemented lazy for only used placeholders
  • Switchable short and long format options, like the load block but customizable. This would be useful e.g. for the net block to hide the ip / ssid and expand on demand.

(I know it's unrelated, but I think it would be a good idea to think about that during the rewrite, if there are no reasons against it. Feel free to move this to another issue to ease the discussion here)

@atheriel
Copy link
Collaborator

A piece that's missing from this discussion is, what are some problems with the existing approach? Can we solve them without async/await? I don't totally understand what the issues are outside of the abstract we-block-on-I/O, and it could be beneficial to list them in this discussion thread.

From an API design side, I tend to think that the omission of generators from the initial async/await implementation limits what we could do for the main update loop, but maybe that's just a lack of imagination on my part.

An alternative might be to try async/await somewhere else: for instance, one place that we tend to proliferate threads is when dealing with D-Bus code. Maybe it's possible to come up with an ergonomic way for blocks to opt-in to a shared D-Bus threadpool using Futures. (We might have to check out the upstream options, of course.)

@greshake
Copy link
Owner Author

Good point.
The goal of any rewrite would be to set i3status-rust up for multiple backends (other than i3status), to increase simplicity, reduce boilerplate and minimize memory operations and increase performance.

Right now, we have a ton of boilerplate, many unnecessary copy's and no flexibility. I'm hoping async IO would also help us streamline a composable, low-overhead rendering pipeline that is also much more stable. Right now, there are many edge-case bugs that can arise from blocking I/O that can't be fixed easily.. Like, block updates that are synced on a shared interval output only a single visual update to i3status for performance, but the timers can be desynced if you, for example, use a custom block with network interaction.
But I totally agree that there is a lot more low-hanging fruit to be had in the current implementation. We could provide a shared futures thread on-demand for blocks that require async IO. And we could use something like this https://docs.rs/calloop/0.4.4/calloop/ to make a rendering pipeline with callbacks and shared references to visual states :)

@greshake
Copy link
Owner Author

greshake commented Aug 22, 2019

We can also take inspiration from some of these projects that have elegant solutions:

@greshake
Copy link
Owner Author

Azul has the architecture that is most similar to ours; https://github.com/maps4print/azul
To solve async issues in azul, they implemented multiple solutions: https://github.com/maps4print/azul/wiki/Timer,-Threads-and-Tasks

@PicoJr
Copy link
Contributor

PicoJr commented May 8, 2020

Maybe it's time to reconsider using async I/O now that async/await is stabilized?

Doing a complete architecture overhaul sounds quite intimidating though.

Maybe we could work out a way to progressively "async" the core by supporting both async and non-async behaviours at the same time?

  • 🧵 = spawns a thread to watch or do something and schedule an update via tx: Sender<Task> if necessary
  • ⚓ = may take some time
Block Behaviour
backlight 🧵
battery 🧵
bluetooth 🧵
cpu
custom 🧵
disk_space
docker ⚓?
focused_window 🧵
ibus 🧵
keyboard_layout 🧵
load
maildir
memory
music 🧵
net
network_manager 🧵
notmuch
nvidia_gpu
pacman
pomodoro
sound 🧵
speedtest 🧵 ⚓?
taskwarrior ⚓?
temperature
time
toggle ⚓?
uptime
watson ⚓?
weather ⚓?
xrandr

I would say most blocks either wait for:

  • 1: their next scheduled update

  • 2: wait for their update logic to run

  • 3: wait in a another thread for something to happen and reschedule an update

  • 1 is fine (performance-wise) for blocks that don't spawn a background thread and have light update logic.
    For blocks relying on a background thread.

  • 2 is fine for light update logic: they won't run (block other updates) for long.
    For blocks relying on a web service (weather) or querying a DB (pacman) it could be an issue.

  • 3 can be a waste of resources if an async interface is available instead.

@ammgws ammgws pinned this issue May 8, 2020
@atheriel atheriel unpinned this issue Jun 24, 2020
@Noah-Kennedy
Copy link

Is anyone working on this? I am bored and would be interested in having a go at progressively making stuff async with tokio.

@PicoJr
Copy link
Contributor

PicoJr commented Sep 2, 2020

Hello, FYI I'm not working on this.

I have mixed feelings about using async inside i3status-rust (only my opinion):

  • most widgets won't benefit from being async
  • mixing threads and async looks daunting to me
  • threads works fine, even spawning a thread per block remains quite cheap

Nevertheless some widgets may benefit from being async (or simply being run inside a thread): pacman, weather, speedtest and maildir...?

@Noah-Kennedy I'll be looking forward reading your code if you decide to give it a go ;)

@kstrafe
Copy link

kstrafe commented Dec 14, 2020

Any updates on this?

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Mar 30, 2021

My attempt to write an async tokio-based version of i3status-rust: https://github.com/MaxVerevkin/swaystatus

Currently there are four blocks: Memory, (slightly simplified) Time, Temperature and sway's keyboard layout.

I like the fact that a (time) block can be as simple as 50 SLOC - https://github.com/MaxVerevkin/swaystatus/blob/master/src/blocks/time.rs

@ammgws
Copy link
Collaborator

ammgws commented Mar 30, 2021

Looks pretty nice!

@MaxVerevkin
Copy link
Collaborator

By the way, if anyone has questions or suggestions regarding the design of swaystatus, don't hesitate to open a new issue, since it's my first attempt at async rust.

@MaxVerevkin
Copy link
Collaborator

By the way, looks like the async implementation uses a little less memory

❯ procs swaystatus --or i3status-rust
 PID:▲  User │ TTY CPU MEM CPU Time │ Command                                                 
             │     [%] [%]          │                                                         
 142363 maxv │     0.0 0.2 00:00:00 │ /home/maxv/rust/i3status-rust/target/release/i3status-rs
 142364 maxv │     0.0 0.1 00:00:00 │ /home/maxv/rust/swaystatus/target/release/swaystatus

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Apr 29, 2021

While porting pomodoro block to swaystatus I thought that it's not very convenient to edit the config file every time you want to change the length of the task or break. So I thought it would be interesting to implement some sort of GUI to set those values.

Thanks to async, it was quite easy to implement this "complex" "graphical user interface" because I didn't have to manually manage the state machine.

So that's what I got: https://imgur.com/a/v8344Ce

The code is fairly simple (it's just the GUI, no functionality yet): https://github.com/MaxVerevkin/swaystatus/blob/c6a55364b28c692964cfb62bbcee0bffbf9140d4/src/blocks/pomodoro.rs

What do you think? Should some other blocks allow this kind of configuration? IMO it's far better than editing the config file (in case of pomodoro).

@alarsyo
Copy link
Contributor

alarsyo commented May 3, 2021

I think this is a step in the right direction, looks awesome @MaxVerevkin !

@ammgws
Copy link
Collaborator

ammgws commented May 5, 2021

What do you think? Should some other blocks allow this kind of configuration? IMO it's far better than editing the config file (in case of pomodoro).

Pinging those who have previously worked on the pomodoro block since you may be interested: @PicoJr @GChicha @kaworu @ghedamat

@ghedamat
Copy link
Contributor

ghedamat commented May 5, 2021

@ammgws @MaxVerevkin thanks for the ping!

This looks great!

How are you planning to persist the selections? should it be until restart so we start from the default but can change it per session?

I'm not familiar with async in rust but I could try to port over your work from swaystatus!

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented May 5, 2021

How are you planning to persist the selections?

I don't plan it (at least for now).

In case of pomodoro, the settings last until the timer is over because the next time I run it I might want to have different settings.

If I wanted to store the settings persistently, I guess I would write into something like ~/.cache/i3status-rs. But for me, it feels weird for a status line generator to have its own persistent storage, I don't know why.

@ghedamat
Copy link
Contributor

ghedamat commented May 5, 2021

that is perfectly fine, one can set the default and then adjust per session

@MaxVerevkin
Copy link
Collaborator

@ghedamat

I'm not familiar with async in rust but I could try to port over your work from swaystatus!

You mean the async or pomodoro in particular? Because I will submit a PR to include async in i3status-rust as soon as all the blocks are ported (current state can be checked here: MaxVerevkin/swaystatus#4) (if you want to get some experience with async rust you can try to port some blocks :) )

one can set the default and then adjust per session

Yeah, that's what I thought too.

@ghedamat
Copy link
Contributor

ghedamat commented May 5, 2021

Oh Async in rust in general! so please submit your pr, I'll def review and if I have extra time maybe port over other blocks!

thanks a lot!

@MaxVerevkin
Copy link
Collaborator

So, one year later... There are two async designs for i3staturs-rs at the moment. I'll leave a short description of both designs here in case anyone wants to do something with it.

1: Block is a Future with an endless loop inside

Branch

speedtest block implementation

A typical block looks something like this:

pub async fn run(config: toml::Value, mut api: CommonApi) -> Result<()> {
    let config = BlocksConfigType::deserialize(config).config_error()?;
    let mut widget = api
        .new_widget()
        .with_format(config.format.with_default("useful default")?);
    loop {
        // Update the state
        let info = get_useful_info().await?;

        widget.set_values(map! {
            /* */
        });
        api.set_widget(&widget).await?;

        // Wait for an event (it can be a DBus signal or whatever)
        select! {
            _ = sleep(config.interval.0) => (),
            UpdateRequest = api.event() => (),
        }
    }
}

Advantages of this design:

  • There is no need maintain block state: the sate machine is generated automatically by the compiler.
  • It's pretty simplistic, at least from my point of view.

2: Update Block trait to be async

Branch

New Block trait definition

speedtest block implementation

A typical block looks something like this:

#[async_trait]
impl Block for BlockType {
    fn interval(&self) -> Option<Duration> {
        Some(self.interval)
    }

    async fn update(&mut self) -> Result<()> {
        let info = get_useful_info().await?;

        self.text.set_texts(self.format.render(&map! {
            /* */
        })?);

        Ok(())
    }

    async fn click(&mut self, e: &I3BarEvent) -> Result<bool> {
        Ok(e.button == MouseButton::Left)
    }

    fn view(&self) -> Vec<&dyn I3BarWidget> {
        vec![&self.text]
    }
}

Advantages of this design:

  • It's easier to port existing blocks.
  • Some might find this design better structured.

@ammgws
Copy link
Collaborator

ammgws commented Jun 19, 2022

Async branch has now been merged into master (still some work to do before release is possible though)

@ammgws ammgws closed this as completed Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants