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

Experimental: Tokio async #1183

Closed
wants to merge 4 commits into from
Closed

Conversation

remi-dupre
Copy link
Contributor

@remi-dupre remi-dupre commented Apr 12, 2021

Hi!

I've spent some time on my evening trying to incorporate tokio's async into this project.

Some of the benefits have already been discussed in #430:

  • no more blocking steps (I actually have a custom block that takes a few seconds waiting for an API's response on my own config, I disabled it as it is quite painful to have the bar occasionally frozen when I click on it)
  • probably helps with the "resourcefriendly" promise of i3status-rust, there is a lot of spawned threads that don't do much
  • this can help removing a lot of logic already abstracted by futures, there is actually a lot of channels everywhere just used to trigger events in every directions, most of is just expressed through an .await when using async. I'm pretty sure the overall code base could be a lot simpler with async and using it to implement high level blocks should not have a very big barrier of entry.

I only implemented a few features for now, but I was interested to share in order to check if some people share my interest. I saw that @MaxVerevkin recently put his own efforts on a similar task but I understand that swaystatus is expected to be a separated project. On the over hand, my approach is to make a small change in the block API:

#[async_trait(?Send)]
pub trait Block {
    // didn't change
    fn id(&self) -> usize;

    // it seemed simpler to handle this when separated from the update logic
    fn update_interval(&self) -> Update {
        Update::Once
    }

    // this is a mix of former `update` and `view`, which now returns owned content
    async fn render(&mut self) -> Result<Vec<Box<dyn I3BarWidget>>>;

    // now this returns a boolean to indicate if an update must be performed
    async fn signal(&mut self, _signal: i32) -> Result<bool> {
        Ok(false)
    }

    // now this returns a boolean to indicate if an update must be performed
    async fn click(&mut self, _event: I3BarEvent) -> Result<bool> {
        Ok(false)
    }
}

First result seem promising, after a few hours of work I managed to have replaced the scheduler with a tiny amount of code, re-implemented the click events and a few blocks (cpu, brightness and custom which can now run a command for a few seconds without blocking the main loop).

Note that I'm not a veteran of Rust async at all, I only experienced it through a few tiny scripts, I have a bit of experience with it in Python though. This means that most of my time was invested in reading tokio and futures documentation and productivity can probably improve a lot.

@MaxVerevkin
Copy link
Collaborator

is expected to be a separated project.

Not really

On the over hand, my approach is to make a small change in the block API:

For me one of the reasons to use async is that the API can be reduced to just one function with a loop inside. You can see how much this can simplfy things in the music block of swaystatus (it's a little bit incomplete, but still).

@remi-dupre
Copy link
Contributor Author

remi-dupre commented Apr 13, 2021

is expected to be a separated project.

Not really

Oh, that would be a bad idea to split efforts then? Do you have an objective with that ? Implementing all blocks (which is were most external help could come I suppose) and propose a switch ?

On the over hand, my approach is to make a small change in the block API:

For me one of the reasons to use async is that the API can be reduced to just one function with a loop inside. You can see how much this can simplfy things in the music block of swaystatus (it's a little bit incomplete, but still).

That's an interesting approach, I'm not a huge fan of how it spawns "never ending tasks that feed outputs in a channel", however I have to agree that it helps with keeping most of the block's state implicit.

With my approach we would still have to keep an explicit struct to hold the state of complex blocks but a lot could be simplified from most blocks by using a global mechanism to handle context (configs and id). It would also be easier to make a finished PR for i3status-rust because of how the API doesn't change much.

@remi-dupre
Copy link
Contributor Author

@MaxVerevkin I also just had this funny thought that it would be possible to write a wrapper for both approach to support the other "syntax". This could help for a transition in whatever direction ^^

@MaxVerevkin
Copy link
Collaborator

I'm not a huge fan of how it spawns "never ending tasks that feed outputs in a channel"

Why? Do you have performance related concerns?

@remi-dupre
Copy link
Contributor Author

remi-dupre commented Apr 13, 2021

I'm not a huge fan of how it spawns "never ending tasks that feed outputs in a channel"

Why? Do you have performance related concerns?

Nope, that's mostly opinionated and your approach might be better.

I'd love to try pushing async to main branch at some point, do you have any plan for that? Would you be interested in putting efforts together?

I'd be willing to completely drop my efforts as I can't really give a big argument in its favor except that it could be easier to transition to it since the API is closer to the sync one, but in the other hand i3status-rust could benefit a lot from a cleaner re-implementation for a lot of its block, this can be a good opportunity :)

@MaxVerevkin
Copy link
Collaborator

do you have any plan for that

Not yet.

Would you be interested in putting efforts together?

Definitely.

@remi-dupre
Copy link
Contributor Author

Would you be interested in putting efforts together?

Definitely.

I'll try to dive deeper into your repo then, I'll put my pride on hold and admit that your approach sounds better. I see that a lot of thinks have already been moved, the only thing that has to be done is implementing new blocks?

do you have any plan for that

Not yet.

My biggest concern is that swaystatus is a whole new repo instead of a branch over i3status-rs (which sounds better? I think the change is not big enough to justify building a separate project?), but I guess that's something that could be dealt with later if we manage to finish the reimplementations.

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Apr 13, 2021

My biggest concern is that swaystatus is a whole new repo instead of a branch over i3status-rs

I made it for a several reasons. For example I didn't want to preserve full compatibility by myself. However I would be happy if it will got merged into i3status-rust with the help of others.

Can I make two forks of one repo on GitHub? If no I'll just create a new branch then.

@MaxVerevkin
Copy link
Collaborator

MaxVerevkin commented Apr 13, 2021

the only thing that has to be done is implementing new blocks?

I think so.

@remi-dupre remi-dupre closed this Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants