Added rayon feature by Julien-cpsn · Pull Request #2 · staticintlucas/stringslice

Julien-cpsn · 2024-08-31T00:07:19Z

Run in parallel

You can have access to parallelized methods by enabling the rayon feature. Thanks to the rayon crate, the string slicing will execute through many threads.

Parallel methods:

par_slice
par_try_slice
par_substring
par_try_substring

Warning

Using the parallel methods on bigger strings is recommended. Parallelism scales greatly on bigger sizes.

staticintlucas

Hey, thanks for the PR!

I've left a few comments, sorry if I'm being a bit nitpicky.

Also please let me know if you have any performance data. I'm super curious to know how long a string would need to be to see any performance improvement.

staticintlucas · 2024-08-31T12:34:56Z


 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+[dependencies]
+rayon_ = { package = "rayon", version = "1.10.0", optional = true }


Can I ask why you're using rayon_ rather than rayon here?

Because dependencies and features can conflict, so you can't name both rayon.

I personally think it's preferable for the user to use rayon and not rayon_

[dependencies] rayon = { ..., optional = true } [features] std = [] rayon = ["std"]

It should be possible to do something like this since Rust creates an implicit feature for optional dependencies.

In Rust 1.60+ you can also use the dep: syntax, but that requires bumping our MSRV

staticintlucas · 2024-08-31T12:38:29Z

+
+[features]
+std = []
+rayon = ["std"]


I know rayon requires std, but do we also need a std feature in this stringslice?

As far as I am aware we should be able to keep stringslice as no_std even if rayon uses std.

I added the std feature for two reasons

so that the user is aware that adding rayon also adds std

it's already present if needed in the future

Ok, sure, makes sense I guess.

In that case should we also make std a default feature?

I don't know, that's on you!

I think adding std to default would be a breaking change, so let's not do that now.

If we need to add it in future I can do that in a 2.0 release.

staticintlucas · 2024-08-31T12:40:13Z

+
+> [!WARNING]
+> Using the **par**allel methods on bigger strings is recommended. Parallelism scales greatly on bigger sizes.
+


Do you have any benchmark info you could add here? I think it would be useful to know how big a string needs to be for the par_* functions to be faster.

I'll try to provide some, yes!

staticintlucas · 2024-08-31T12:44:14Z

+
+#[cfg(feature = "std")]
+extern crate std;


Suggested change

#[cfg(feature = "std")]

extern crate std;

This shouldn't be necessary

First I did like #2 (comment)

But then I encountered some problems with tests, so that's how I figured it out. Maybe there's a better solution

Ah, yeah, for tests you need std.

You should do something like #[cfg_attr(not(any(feature = "std", test)), no_std)] to enable std for tests

staticintlucas · 2024-08-31T12:45:12Z

+#![cfg(not(feature = "std"))]
 use core::ops::{Bound, RangeBounds};

+#[cfg(feature = "std")]
+use std::ops::{Bound, RangeBounds};


Suggested change

#![cfg(not(feature = "std"))]

use core::ops::{Bound, RangeBounds};

#[cfg(feature = "std")]

use std::ops::{Bound, RangeBounds};

use core::ops::{Bound, RangeBounds};

You should be able to use core::* even with std

The rust docs say that it's better to use std instead of core, but it doesn't elaborate on that

As a compromise you should be able to do something like this to avoid duplicate imports:

#[cfg(not(feature = "std")) use core as std; use std::whatever;

staticintlucas · 2024-08-31T12:58:51Z

+    #[cfg(feature = "rayon")]
+    /// Returns a string slice for the given range of characters
+    ///
+    /// This method will panic if the range is invalid,
+    /// for example if the beginning is greater than the end.
+    ///
+    /// Runs in parallel
+    ///
+    /// # Examples
+    /// ```
+    /// use stringslice::StringSlice;
+    ///
+    /// assert_eq!("Ùníc😎de".slice(4..5), "😎");
+    /// ```
+    fn par_slice(&self, range: impl RangeBounds<usize>) -> &str;
+
+    #[cfg(feature = "rayon")]
+    /// Returns an [`Option`] containing string slice for the given range of characters
+    ///
+    /// This method will return [`None`] if the range is invalid,
+    /// for example if the beginning is greater than the end.
+    ///
+    /// Runs in parallel
+    ///
+    /// # Examples
+    /// ```
+    /// use stringslice::StringSlice;
+    ///
+    /// assert_eq!("Ùníc😎de".try_slice(4..5), Some("😎"));
+    /// ```
+    /// [`Option`]: https://doc.rust-lang.org/std/option/enum.Option.html
+    /// [`None`]: https://doc.rust-lang.org/std/option/enum.Option.html#variant.None
+    fn par_try_slice(&self, range: impl RangeBounds<usize>) -> Option<&str>;
+
+    #[cfg(feature = "rayon")]
+    /// Returns a string slice between the given beginning and end characters
+    ///
+    /// This method will panic if the parameters are invalid,
+    /// for example if the beginning is greater than the end.
+    ///
+    /// Runs in parallel
+    ///
+    /// # Examples
+    /// ```
+    /// use stringslice::StringSlice;
+    ///
+    /// assert_eq!("Ùníc😎de".substring(4, 5), "😎");
+    /// ```
+    fn par_substring(&self, begin: usize, end: usize) -> &str;
+
+    #[cfg(feature = "rayon")]
+    /// Returns an [`Option`] containing string slice between the given beginning and end characters
+    ///
+    /// This method will return [`None`] if the parameters are invalid,
+    /// for example if the beginning is greater than the end.
+    ///
+    /// Runs in parallel
+    ///
+    /// # Examples
+    /// ```
+    /// use stringslice::StringSlice;
+    ///
+    /// assert_eq!("Ùníc😎de".try_substring(4, 5), Some("😎"));
+    /// ```
+    /// [`Option`]: https://doc.rust-lang.org/std/option/enum.Option.html
+    /// [`None`]: https://doc.rust-lang.org/std/option/enum.Option.html#variant.None
+    fn par_try_substring(&self, begin: usize, end: usize) -> Option<&str>;


Rather than adding these methods to the existing StringSlice trait, do you think it would be cleaner to create a new ParStringSlice trait for these?

That could be great yes

Added rayon feature

4f97387

staticintlucas reviewed Aug 31, 2024

View reviewed changes


		> [!WARNING]
		> Using the parallel methods on bigger strings is recommended. Parallelism scales greatly on bigger sizes.

Conversation

Julien-cpsn commented Aug 31, 2024

Run in parallel

Uh oh!

staticintlucas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Julien-cpsn Aug 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Julien-cpsn Aug 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Julien-cpsn Aug 31, 2024 •

edited

Loading

Julien-cpsn Aug 31, 2024 •

edited

Loading