Optimize strip_prefix and strip_suffix with str patterns #69784

benesch · 2020-03-06T20:18:30Z

As mentioned in #67302 (comment).
I'm not sure whether adding these methods to Pattern is desirable—but they have default implementations so the change is backwards compatible. Plus it seems like they're slated for wholesale replacement soon anyway? #56345

Constructing a Searcher in strip_prefix and strip_suffix is
unnecessarily slow when the pattern is a fixed-length string. Add
strip_prefix and strip_suffix methods to the Pattern trait, and add
optimized implementations of these methods in the str implementation.
The old implementation is retained as the default for these methods.

rust-highfive · 2020-03-06T20:18:34Z

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

mlodato517

Just asking some questions because I'm new to rust but want to learn and get more involved! :-)

mlodato517 · 2020-03-10T18:38:32Z

src/libcore/str/mod.rs

@@ -3984,26 +3984,15 @@ impl str {
    /// ```
    /// #![feature(str_strip)]
    ///
-    /// assert_eq!("foobar".strip_prefix("foo"), Some("bar"));
-    /// assert_eq!("foobar".strip_prefix("bar"), None);
+    /// assert_eq!("foo:bar".strip_prefix("foo:"), Some("bar"));


Just wondering - do these changes improve the test cases or are they simply to make the docs more readable? Or some other thing? I assume it's not because the previous tests failed with the new implementation.

They improve the test cases to account for a bug that I introduced while rewriting these methods that was not properly caught by the old test cases. The old test cases had the defect that they split the string exactly in half. My buggy implementation for strip_suffix sliced from s[pat.len()..] when it needed to slice from s[s.len() - pat.len()..], which was not caught by any of the previous test cases.

mlodato517 · 2020-03-10T18:39:50Z

src/libcore/str/mod.rs

-    /// assert_eq!("foobar".strip_prefix("foo"), Some("bar"));
-    /// assert_eq!("foobar".strip_prefix("bar"), None);
+    /// assert_eq!("foo:bar".strip_prefix("foo:"), Some("bar"));
+    /// assert_eq!("foo:bar".strip_prefix("bar"), None);
    /// assert_eq!("foofoo".strip_prefix("foo"), Some("foo"));


Should this be updated to foo:foo? I guess this partly depends on the comment above :-)

The intent of this test case, as best as I can tell, is to check that strip_prefix(P) only strips one prefix from a string like PPPP, so adding the colon would change what this test is intended to do.

mlodato517 · 2020-03-10T18:50:00Z

src/libcore/str/mod.rs

@@ -3041,7 +3041,7 @@ impl str {
    /// ```
    #[stable(feature = "rust1", since = "1.0.0")]
    pub fn starts_with<'a, P: Pattern<'a>>(&'a self, pat: P) -> bool {
-        pat.is_prefix_of(self)
+        pat.strip_prefix_from(self).is_some()


I think I'm missing something :-( - what is this change for?

Oops, it's errant. Thanks. It's from an earlier revision of the PR where I was experimenting with removing the is_prefix_of and is_suffix_of methods and rewriting them in terms of the new strip_prefix_from and strip_suffix_from methods.

mlodato517 · 2020-03-10T19:15:12Z

src/liballoc/string.rs

+
+    #[inline]
+    fn strip_prefix_from(self, haystack: &'a str) -> Option<&'a str> {
+        self[..].strip_prefix_from(haystack)


So I'm having a little trouble following the flow of these methods. I'm going to botch some made up syntax and pretend Module.method is the method method on an instance of the Module struct but can you tell me if this what's going on?

We have

String.strip_prefix_from which calls str.strip_prefix_from which is an implementation of Pattern.strip_prefix_from optimized because we know the length of the String instance

str.strip_prefix which calls prefix.strip_prefix_from(self) which will call the optimized implementation if prefix is a str or char

char.strip_prefix_from which is an optimized implementation of Pattern.strip_prefix_from

a default implementation of Pattern.strip_prefix_from which is for ... I don't know 🤔 The PR description says "they have default implementations so the change is backwards compatible" - what does that mean? That sounds like, "if we no longer override strip_prefix_from for str or char they'll get the default implementation"? Or am I reading that wrong?

Yep, exactly right! (4) is for anyone who wants to implement Pattern for their own type (say, a function) but doesn't want to go to the trouble of implementing a specialized strip_prefix_from or strip_suffix_from method—or perhaps their pattern is not of a fixed length and therefore there isn't a way to optimize the method beyond constructing and using a full search.

Ohh okay I understand now! I was originally confused because I didn't notice that, if there wasn't a default implementation, implementors would have to supply their own implementation. Forgot that optional methods without defaults aren't things :-) Thanks!

mlodato517

I don't see any "Resolve Conversation" button, but you resolved all my comments, thank you :-)
Just one more question and then this is ✅ from my perspective but I have no idea what I'm talking about so I'd rather someone else review this

mlodato517 · 2020-03-10T20:59:58Z

src/libcore/str/pattern.rs

@@ -47,6 +47,22 @@ pub trait Pattern<'a>: Sized {
        matches!(self.into_searcher(haystack).next(), SearchStep::Match(0, _))
    }

+    /// Removes the pattern from the front of haystack, if it matches.
+    #[inline]
+    fn strip_prefix_from(self, haystack: &'a str) -> Option<&'a str> {


Does this method no longer have tests? Do we want it to still have tests?

bors · 2020-03-13T22:28:55Z

☔ The latest upstream changes (presumably #69986) made this pull request unmergeable. Please resolve the merge conflicts.

tesuji · 2020-03-23T00:36:44Z

@benesch You need to rebase this PR.

benesch · 2020-03-28T18:57:50Z

Thanks, done.

src/libcore/str/pattern.rs

Constructing a Searcher in strip_prefix and strip_suffix is unnecessarily slow when the pattern is a fixed-length string. Add strip_prefix and strip_suffix methods to the Pattern trait, and add optimized implementations of these methods in the str implementation. The old implementation is retained as the default for these methods.

kennytm · 2020-03-30T18:38:34Z

@bors r+

bors · 2020-03-30T18:38:35Z

📌 Commit ac478f2 has been approved by kennytm

bors · 2020-03-30T23:10:51Z

⌛ Testing commit ac478f2 with merge 23e4949bb9f13174569c3f369434398db395ee16...

bors · 2020-03-30T23:14:38Z

💔 Test failed - checks-azure

benesch · 2020-03-31T00:24:55Z

This seems to have been part of a Bors batch that was canceled? https://dev.azure.com/rust-lang/rust/_build/results?buildId=24890&view=results

Will it be requeued automatically or does it need another r+?

kennytm · 2020-03-31T07:41:18Z

@bors retry

…=kennytm Optimize strip_prefix and strip_suffix with str patterns As mentioned in rust-lang#67302 (comment). I'm not sure whether adding these methods to `Pattern` is desirable—but they have default implementations so the change is backwards compatible. Plus it seems like they're slated for wholesale replacement soon anyway? rust-lang#56345 ---- Constructing a Searcher in strip_prefix and strip_suffix is unnecessarily slow when the pattern is a fixed-length string. Add strip_prefix and strip_suffix methods to the Pattern trait, and add optimized implementations of these methods in the str implementation. The old implementation is retained as the default for these methods.

@ghost

Rollup of 9 pull requests Successful merges: - rust-lang#69784 (Optimize strip_prefix and strip_suffix with str patterns) - rust-lang#70548 (Add long error code for error E0226) - rust-lang#70555 (resolve, `try_resolve_as_non_binding`: use `delay_span_bug` due to parser recovery) - rust-lang#70561 (remove obsolete comment) - rust-lang#70562 (infer array len from pattern) - rust-lang#70585 (std: Fix over-aligned allocations on wasm32-wasi) - rust-lang#70587 (Add `Rust` to the code snippet) - rust-lang#70588 (Fix incorrect documentation for `str::{split_at, split_at_mut}`) - rust-lang#70613 (more clippy fixes) Failed merges: r? @ghost

benesch · 2020-03-31T22:03:49Z

Thanks very much, everyone!

rust-highfive assigned kennytm Mar 6, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 6, 2020

benesch mentioned this pull request Mar 6, 2020

tracking issue for strip_prefix, strip_suffix for str #67302

Closed

mlodato517 reviewed Mar 10, 2020

View reviewed changes

benesch force-pushed the fast-strip-prefix-suffix branch from ee53aa0 to b00a211 Compare March 10, 2020 20:30

mlodato517 reviewed Mar 10, 2020

View reviewed changes

kennytm approved these changes Mar 13, 2020

View reviewed changes

benesch force-pushed the fast-strip-prefix-suffix branch 2 times, most recently from 0b86e69 to dbbea25 Compare March 28, 2020 18:39

tesuji reviewed Mar 29, 2020

View reviewed changes

src/libcore/str/pattern.rs Outdated Show resolved Hide resolved

src/libcore/str/pattern.rs Outdated Show resolved Hide resolved

benesch force-pushed the fast-strip-prefix-suffix branch from dbbea25 to ac478f2 Compare March 30, 2020 15:10

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 30, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 30, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2020

Centril mentioned this pull request Mar 31, 2020

Rollup of 8 pull requests #70601

Closed

Centril mentioned this pull request Mar 31, 2020

Rollup of 9 pull requests #70609

Closed

Centril mentioned this pull request Mar 31, 2020

Rollup of 9 pull requests #70617

Merged

bors merged commit 9ee373f into rust-lang:master Mar 31, 2020

benesch deleted the fast-strip-prefix-suffix branch March 31, 2020 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize strip_prefix and strip_suffix with str patterns #69784

Optimize strip_prefix and strip_suffix with str patterns #69784

benesch commented Mar 6, 2020 •

edited

Loading

rust-highfive commented Mar 6, 2020

mlodato517 left a comment

mlodato517 Mar 10, 2020

benesch Mar 10, 2020

mlodato517 Mar 10, 2020

benesch Mar 10, 2020

mlodato517 Mar 10, 2020

benesch Mar 10, 2020

mlodato517 Mar 10, 2020

benesch Mar 10, 2020

mlodato517 Mar 10, 2020

benesch Mar 10, 2020

mlodato517 left a comment

mlodato517 Mar 10, 2020

bors commented Mar 13, 2020

tesuji commented Mar 23, 2020

benesch commented Mar 28, 2020

kennytm commented Mar 30, 2020

bors commented Mar 30, 2020

bors commented Mar 30, 2020

bors commented Mar 30, 2020

benesch commented Mar 31, 2020

kennytm commented Mar 31, 2020

benesch commented Mar 31, 2020

Optimize strip_prefix and strip_suffix with str patterns #69784

Optimize strip_prefix and strip_suffix with str patterns #69784

Conversation

benesch commented Mar 6, 2020 • edited Loading

rust-highfive commented Mar 6, 2020

mlodato517 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlodato517 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 13, 2020

tesuji commented Mar 23, 2020

benesch commented Mar 28, 2020

kennytm commented Mar 30, 2020

bors commented Mar 30, 2020

bors commented Mar 30, 2020

bors commented Mar 30, 2020

benesch commented Mar 31, 2020

kennytm commented Mar 31, 2020

benesch commented Mar 31, 2020

benesch commented Mar 6, 2020 •

edited

Loading