search

johannesvollmer / exrs

100% Safe Rust OpenEXR file library
Other
153 stars 24 forks source link

add rayon feature #241

Closed oligamiq closed 2 weeks ago

fintelia commented 1 month ago

Not a maintainer, but FYI this is an API breaking change because any downstream code relying on the now cfg'd out methods will no longer compile. If rayon was left as a default feature, this change could be done in a backwards compatible way

oligamiq commented 1 month ago

All the tests have passed, but indeed, this might be a breaking change.

johannesvollmer commented 1 month ago

Thanks for taking the time to contribute! The changed you proposed seem solid. I would have solved it differently, but I don't have the time to do that:

As the calls to rayon are quite few and isolated, it should be possible to keep the API as-is. Then at runtime falling back to sequential processing if the rayon feature is disabled. This would require some more complicated code changed, but it would avoid us having to sprinkle cfg attributes in multiple unrelated code sections, which seems hard to maintain in my eyes (and is not backwards compatible).

The backwards incompatible changes would require major version upgrade, exr 2.0, which makes it harder for people to use the newest versions, as they have to manually update the version in their projects.

What do you think?

oligamiq commented 1 month ago

I think it’s possible to maintain all the public structs and APIs while marking them as deprecated and falling back. I'll be busy for a few days, but after that, I can give it a try if you'd like.

johannesvollmer commented 1 month ago

Checked the exr code and found something problematic, there is at least one public function that exposes a rayon item at compile time: pub fn new_with_thread_pool.

Someone might rely on this function. As it references the rayon thread pool in the function type signature, it would no longer compile if rayon is removed. This means we cannot get around a breaking change. Please correct me if I'm wrong.

Maybe it would be ok if the default is to enable rayon? Then the breaking change would be opt-in.

oligamiq commented 1 month ago

That's right. I share the same opinion. While it's possible to replace the dependent types with generics, it would cause some significant breaking changes. In the case of removing Rayon from image-rs, it's acceptable to have Rayon set to default ON. However, setting Rayon to default ON is generally not advisable.

oligamiq commented 1 month ago

Since there seem to be a lot of issues left, how about removing rayon from the default feature list at the time of a major version update?

johannesvollmer commented 3 weeks ago

after thinking about this, I'm happy to merge this. (of course we need to fix the 3 small conflicts, but that's not a big deal.) thanks for your contribution!

johannesvollmer commented 3 weeks ago

@fintelia (edit:) I see that image-rs uses rayon, but only with a flag. I assume image will pass this flag to the exrs dependency?

fintelia commented 3 weeks ago

Yes. After this lands, image will switch to only enabling the rayon feature on exr when its own rayon feature is enabled

oligamiq commented 2 weeks ago

I accepted the CI-related reviews and resolved the conflicts.