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

Port tower to futures-lite #768

Open
notgull opened this issue Apr 19, 2024 · 6 comments
Open

Port tower to futures-lite #768

notgull opened this issue Apr 19, 2024 · 6 comments
Labels
C-musing Category: musings about a better world

Comments

@notgull
Copy link

notgull commented Apr 19, 2024

futures-lite is a subset of the futures-util crate that contains much less bloat and compiles an order of magnitude faster. I would like to look into replacing some of futures with futures-lite to reduce the number of dependencies tower has.

The issue is that futures-util is currently exposed in some areas in the public API, so this would be a breaking change. I would replace these public types with opaque ones; I just want to make sure everyone is fine with that.

cc tokio-rs/axum#2469

@tobz
Copy link
Member

tobz commented Jul 21, 2024

@notgull I think this would be fine work on a PR for if you're still interested. From my cursory scan, it seems like the public changes would be minimal, and it might be nice to see the full changeset to potentially decide that maybe we want to actually use our own wrapper types to entirely decouple the public API from third-party deps, etc.

@tobz tobz added the C-musing Category: musings about a better world label Jul 21, 2024
@GlenDC
Copy link
Contributor

GlenDC commented Jul 21, 2024

I'll take this one if @notgull does not mind, I can imagine he has already plenty on his plate with smol and the like

@GlenDC
Copy link
Contributor

GlenDC commented Jul 21, 2024

Question: there are some dev usages (in tests/examples) where we use features not available in futures_lite. E.g. we use https://docs.rs/futures/latest/futures/channel/mpsc/index.html for some examples/tests. Easiest would be to use tokio's mpsc for that given those examples use tokio already anyway?

Ok so that's easy to solve. The bigger issues I see with stuff that's not available in futures_lite:

  • TryFutureExt
  • TryStreamExt
  • Either

What to do with these? Implement these in tower?

@notgull
Copy link
Author

notgull commented Jul 21, 2024

I'll take this one if @notgull does not mind, I can imagine he has already plenty on his plate with smol and the like

By all means go ahead. Following some IRL events I haven't had much mojo left over for open source in a while.

  • TryFutureExt

  • TryStreamExt

  • Either

I don't these these would be too nontrivial to implement here.

@tobz
Copy link
Member

tobz commented Jul 21, 2024

TryFutureExt/TryStreamExt could totally be implemented in the crate, sure, but it's a lot of work for what seems like a questionable benefit.

Now that I look more at it, it seems like we're already using a fairly stripped down set of features (just alloc) for futures-util, so it should be pulling in minimal dependencies. As much as I personally find myself saying "ughhh" when people ask me to show numbers... it does seem like it would be worth it here to try and show a quick example, if possible, of how much faster futures-lite compiles than futures-core + futures-task + futures-util.

@GlenDC
Copy link
Contributor

GlenDC commented Jul 22, 2024

Even if it is a bit faster it would still mean you need to re implement some of the missing features directly in tower, which feels out of scope to me. That's really something that should be in some futures-lite-util crate, which already exists but is not "official" (I think?) and does not contain these.

In plabayo/rama I did manage to fully migrate to futures-lite, but only because I managed to mostly use async await everywhere, not requiring any of these extensions. On top of that some stuff that futures is now used for (e.g. Ready/ready and pin) you can now use the std library for AFAIK, so those can be here also simply replaced with std::future::{Ready, ready} and std::pin::pin. Unless I miss something.

But that said, having worked now a bit on this issue, I do start to wonder what's the point. Might as well just use futures for now. If there is a ever a future where you switch to async traits (like I did for async-tower and the ported tower into rama, or where you can use impl traits for associated trait types, then we can drop it same way as I did, as those use cases could be implemented using async await. Until then, I would just continue to use futures. An additional reason is that most big projects will anyway have futures already in their dep tree as an indirect dep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-musing Category: musings about a better world
Projects
None yet
Development

No branches or pull requests

3 participants