Added rayon feature #2

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

Cargo.toml

@@ -11,6 +11,15 @@ keywords = ["string", "slice", "substring", "unicode", "utf-8"]
 categories = ["no-std", "algorithms", "parsing", "rust-patterns", "text-processing"]

 # 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

Cargo.toml

+
+[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

README.md

+
+> [!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

src/lib.rs

+
+#[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

src/lib.rs

+#![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;

src/lib.rs

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

src/lib.rs

+    #[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

src/lib.rs

Added rayon feature

4f97387

staticintlucas reviewed Aug 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added rayon feature #2

Added rayon feature #2

Julien-cpsn commented Aug 31, 2024

staticintlucas left a comment

staticintlucas Aug 31, 2024

Julien-cpsn Aug 31, 2024 •

edited

Loading

staticintlucas Aug 31, 2024

staticintlucas Aug 31, 2024

Julien-cpsn Aug 31, 2024

staticintlucas Aug 31, 2024

Julien-cpsn Aug 31, 2024

staticintlucas Aug 31, 2024

staticintlucas Aug 31, 2024

Julien-cpsn Aug 31, 2024 •

edited

Loading

staticintlucas Aug 31, 2024

Julien-cpsn Aug 31, 2024

staticintlucas Aug 31, 2024

staticintlucas Aug 31, 2024

Julien-cpsn Aug 31, 2024

staticintlucas Aug 31, 2024

staticintlucas Aug 31, 2024

Julien-cpsn Aug 31, 2024


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

Added rayon feature #2

Are you sure you want to change the base?

Added rayon feature #2

Conversation

Julien-cpsn commented Aug 31, 2024

Run in parallel

staticintlucas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julien-cpsn Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julien-cpsn Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julien-cpsn Aug 31, 2024 •

edited

Loading

Julien-cpsn Aug 31, 2024 •

edited

Loading