Skip to content

breaking: directly depend on crossbeam-channel instead of pulling in all of crossbeam #69

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

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

Conversation

m4rch3n1ng
Copy link
Contributor

Warning

this is a breaking change, since you have previously been re-exporting crossbeam.

termimad does not actually need most of the features of crossbeam, it only needs its channel. so you can save on a few unneeded dependencies if you just depend on the crossbeam-channel directly.

at this point it might be worth it to consider just using the std mpsc channel (or putting crossbeam behind a feature flag), as since rust 1.67 the std mpsc and the crossbeam channel are mostly the same performance wise and the extra functionality that crossbeam provides is, as far as i can tell, not used here.

@Canop
Copy link
Owner

Canop commented Apr 20, 2025

What you say is reasonnable.

I have still to try estimate the consequences for applications using the reexported crossbeam. Many of them use the select! macro, for exemple, and mpmc capabilities.

Did you measure a gain (binary size, perhaps) ?

@m4rch3n1ng
Copy link
Contributor Author

I have still to try to estimate the consequences for applications using the reexported crossbeam.

i think it's fair to expect people that need crossbeam to be able to import it themselves. i personally am against exporting entire crates entirely (with a few exceptions like for macros, but i think those should be marked as doc(hidden)), as that can cause a lot more breaking changes than necessary. if crossbeam has a breaking 0.9 release, then you have no way of cleanly upgrading to that version without having to release a breaking change yourself. that's why i would actually advocate to completely remove the re-export of crossbeam-channel and just ask users to add the dependency themselves.

i would also still advocate for putting the crossbeam dependency behind an optional feature and to just rely on the std mpsc by default.

Did you measure a gain (binary size, perhaps)

i don't really care about binary size (unless it gets concerningly big, which i highly doubt would be the fault of crossbeam), but i mostly did this for compile time. on a clean project i can consistently get ~1s of speedup (~11s -> ~10s) for a clean build in --release and ~.5s of speedup for a debug build (~6.5s -> ~6.0). on a larger project like steel the speedup is almost entirely negligible and probably just noise (although i still think it is unnecessary ).

@m4rch3n1ng
Copy link
Contributor Author

i also wanted to say that this is not time-sensitive at all. if you want to wait for something else that warrants a breaking change anyway - something like a new crossterm version - and only then merge this, then that would be fine by me too, as this is not actually a change in any functionality.

@Canop
Copy link
Owner

Canop commented Apr 20, 2025

i think it's fair to expect people that need crossbeam to be able to import it themselves

The point of reexporting the crate is to prevent version conflicts. That's why this reexport and the one of crossterm were required. I'm aware there are also downsides and that it's a balance to find, but it's not something that can be changed lightly.

@m4rch3n1ng m4rch3n1ng force-pushed the crossbeam-to-crossbeam-channel branch from 2eb91f5 to e40db43 Compare May 16, 2025 00:18
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.

2 participants