kornelski / rust-rgb

struct RGB for sharing pixels between crates
https://lib.rs/rgb
MIT License
97 stars 19 forks source link

Remove feature flags for extra types #73

Closed ripytide closed 1 month ago

ripytide commented 3 months ago

As discussed in https://github.com/kornelski/rust-rgb/issues/39#issuecomment-753418889

kornelski commented 3 months ago

The alt module is a bit arbitrary, so it could be removed — all pixel types could be in the root.

However, I think it's still worth having feature flags for less common pixel layouts. Cost of every function will be multiplied by the number of pixel types in the crate, and people will be making requests for more and more weird combinations.

I don't want other users to worry about "bloat" in this crate, whether it's a real bloat, or only perceived.

ripytide commented 3 months ago

Cost of every function will be multiplied by the number of pixel types in the crate

I'm confused how this would be the case, which functions are based on the number of defined pixels? Wait, do you mean the macros?

It is also worth me stating some of the disadvantages of feature-ification.

Is it worth running some compile-time benchmarks on this crate to test the difference feature-ification actually makes? That way we can put a number on it and make an informed decision?

kornelski commented 3 months ago

If you decide to add a function like fn red(&self), then for 10 pixel layouts it's implemented 10 times.

It's even worse for impl From<RGB> for BGR for every combination of the types, since that's quadratic.

ripytide commented 3 months ago

Ah I see, you mean the monomorphization grows linearly to the number of pixel types. That makes sense, although theoretically unused implementations and function variations would be optimized away but I don't know how intelligent the compiler is about that.

Which I suppose is another reason it might be worth testing this with the current compiler.

ripytide commented 3 months ago

I think it's also worth discussing another alternative option which would be to feature gate every pixel variation. Adding rgb and bgr features could make the crate more flexible to users who only want to use a specific non-Rgb pixel type. Or maybe a user only wants the Pixel trait and helper methods for their own custom pixel types.

This would be more consistent and so could be easier to debug for library consumer.

ripytide commented 2 months ago

I've attempted feature flagging all pixels except Rgb and Rgba under an alt feature and testing the compile time differences. Done in the last commit of https://github.com/ripytide/rust-rgb/tree/feature-test This was a really horrible change to make to the code of the library as it adds #[cfg()] annotations absolutely everywhere which was very unpleasant to implement.

Here are my results for clean builds:


cargo build -r = 0.20s
cargo build = 0.24s

cargo build -r -F "alt" = 0.36s
cargo build -F "alt" = 0.44s

cargo build -r -F "legacy num-traits defmt-03 serde bytemuck" = 5.98s
cargo build -F "legacy num-traits defmt-03 serde bytemuck" = 6.16s

cargo build -r -F "legacy num-traits defmt-03 serde bytemuck alt" = 6.24s
cargo build -F "legacy num-traits defmt-03 serde bytemuck alt" = 6.30s
ripytide commented 2 months ago

Personally I don't think the mild compile-time benefits or the potential perceived "bloat" by users is worth the "featureification" of the code-base, especially if we trust the compiler to optimize out most unused code.

There is also a case to be made that features make the library harder to use for those using non-standard formats as they have to figure out which features to turn on which isn't a problem if we don't feature gate them.