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

Add multithreading support to Multi* geometries #1265

Merged
merged 8 commits into from
Nov 6, 2024

Conversation

urschrei
Copy link
Member

@urschrei urschrei commented Nov 6, 2024

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

I think I've set up the Cargo features correctly – the multithreading feature is disabled by default in geo-types, but enabled by geo when geo's default features are active. I just want to make sure the other geo-types default features aren't impacted.

Closes #1257.

This PR should merge before #1246.

Copy link
Member

@frewsxcv frewsxcv left a comment

Choose a reason for hiding this comment

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

Can you also update the doc comment about the multithreading feature flag in geo/lib.rs? To mention these new features functionality, and/or indicate it's not just for rstar

@urschrei urschrei marked this pull request as ready for review November 6, 2024 19:15
geo-types/src/lib.rs Outdated Show resolved Hide resolved
}
}

#[cfg(feature = "multithreading")]
Copy link
Member

Choose a reason for hiding this comment

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

The consuming into implementation makes sense to me.

And I can see how the ref and mut flavors below provide necessary non-consuming functionality. But what does it look like to use it? Can you include some example code in your PR that uses these new methods to make the new interface easier to evaluate?

My guess is that you'd need to do something like

(& my_multi_line_string).into_par_iter()
and
(&mut my_multi_line_string).into_par_iter()
?

Which is a bit rough. I'm afraid people will just end up my_multi_line_string.clone().into_par_iter() out of confusion.

Would it get better if we got rid of these two ref/mut implementations and instead implemented rayon::IntoParallelRefIterator and rayon::IntoParallelRefMutIterator which seem purpose built for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm. These work with the current impls:

    #[test]
    fn test_multithreading_linestring() {
        let multi: MultiLineString<i32> = wkt! {
            MULTILINESTRING((0 0,2 0,1 2,0 0), (10 10,12 10,11 12,10 10))
        };
        let mut multimut: MultiLineString<i32> = wkt! {
            MULTILINESTRING((0 0,2 0,1 2,0 0), (10 10,12 10,11 12,10 10))
        };
        let _ = multi.par_iter().for_each(|_p| ());
        let _ = multimut.par_iter_mut().for_each(|_p| ());
        let _ = &multi.into_par_iter().for_each(|_p| ());
        let _ = &mut multimut.par_iter_mut().for_each(|_p| ());
    }

I admittedly didn't see the rayon::IntoParallelRefIterator and just adapted our existing iterators to rayon::IntoParallelIterator, but I'm confused by

This trait is automatically implemented for I where &I: IntoParallelIterator. In most cases, users will want to implement IntoParallelIterator rather than implement this trait directly.

Copy link
Member

Choose a reason for hiding this comment

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

Oh splendid! They're already implemented by the blanket impl!

I'm glad that users will be able to do:

let _ = multi.par_iter().for_each(|_p| ());
let _ = multimut.par_iter_mut().for_each(|_p| ());

vs. the slightly more arcane:

let _ = &multi.into_par_iter().for_each(|_p| ());
let _ = &mut multimut.into_par_iter().for_each(|_p| ());

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah the latter two were just to make sure "everything" worked. Thanks for the sanity check!

Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

Feature flags LGTM!

@urschrei urschrei added this pull request to the merge queue Nov 6, 2024
Merged via the queue into georust:main with commit cf020af Nov 6, 2024
18 checks passed
@urschrei urschrei deleted the multi_rayon branch November 6, 2024 20:54
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.

Multi-thread support for geo-types using Rayon
3 participants