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

Clean up gating on concurrent feature #140

Merged
merged 2 commits into from
Nov 25, 2023

Conversation

JoeyBF
Copy link
Collaborator

@JoeyBF JoeyBF commented Nov 18, 2023

I was able to contain all the gates in a new maybe-rayon crate. When concurrency is enabled, it is a thin wrapper around the rayon API, or at least the part of the API that the rest of the codebase uses. When it is disabled, the crate provides the same API except that the implementation is completely serial, and doesn't depend on rayon.

Every feature gate is now either in maybe-rayon, in some comments in the steenrod example, or in the resolve_concurrent benchmark.

By unifying some parts of the code, it happens that serial operations now needlessly communicate via mpsc channels. Also, secondary computations now unconditionally compute intermediates upfront, but that's simply a reordering of the same operations as previously. As far as I can tell, performance is unchanged.

@JoeyBF JoeyBF merged commit 8075e0a into SpectralSequences:master Nov 25, 2023
25 checks passed
@JoeyBF JoeyBF deleted the clean_gates branch November 25, 2023 04:38
impl<I: IndexedParallelIterator> MaybeIndexedParallelIterator for I {}

impl<I: IntoParallelIterator> MaybeIntoParallelIterator for I {
fn maybe_into_par_iter(self) -> Self::Iter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe_par should stay together since this unconditionally is turning it into a maybe parallel iterator:

Suggested change
fn maybe_into_par_iter(self) -> Self::Iter {
fn into_maybe_par_iter(self) -> Self::Iter {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that's a good point, I'll fix that. Thanks!

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.

3 participants