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

Rework of FindSubstring using iterator instead #565

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fflorent
Copy link
Contributor

Use an iterator of Memchr for FindSubstring, accordingly to the discussion in #507.

Don't hesitate to bikeshed. Also I won't be offended if you don't feel that's worthy :).

Florent

@coveralls
Copy link

coveralls commented Aug 20, 2017

Coverage Status

Coverage decreased (-0.009%) to 88.067% when pulling 73021eb on fflorent:memchr_change_proposal into 460041a on Geal:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.009%) to 88.067% when pulling 73021eb on fflorent:memchr_change_proposal into 460041a on Geal:master.

@fflorent
Copy link
Contributor Author

@Hywan In order to check how fast is this implementation compared to the previous one, do you still have the bench test for this #507 (comment)?

Thanks in advance :)

@Hywan
Copy link
Contributor

Hywan commented Aug 21, 2017

@fflorent No I don't, sorry :-(.

src/traits.rs Outdated
@@ -467,25 +467,18 @@ impl<'a,'b> FindSubstring<&'b [u8]> for &'a[u8] {
memchr::memchr(substr[0], self)
} else {
let max = self.len() - substr_len;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add +1 directly to the definition of max.

src/traits.rs Outdated
}

None
let haystack = &self[..max + 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

And then remove +1 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

@fflorent
Copy link
Contributor Author

I have committed a banchmark in specific.rs for take_until_and_consume.

Here are the results:

-- before
running 1 test
test take_until_and_consume_bench ... bench:         272 ns/iter (+/- 6)

-- after
running 1 test
test take_until_and_consume_bench ... bench:         310 ns/iter (+/- 3)

So… not really good I would say. There might be tricks to catch up the previous performance, I am doing tests.

@coveralls
Copy link

coveralls commented Aug 21, 2017

Coverage Status

Coverage increased (+0.04%) to 88.118% when pulling 6d00f95 on fflorent:memchr_change_proposal into 460041a on Geal:master.

@fflorent
Copy link
Contributor Author

A quick look didn't give me any hint on how to keep the optimization. I guess such a gap of performance between the previous solution and this proposal excludes the latter.

May I close the PR? Or do you (@Hywan, @Geal) have some ideas on your side?

@coveralls
Copy link

coveralls commented Aug 21, 2017

Coverage Status

Coverage increased (+0.04%) to 88.118% when pulling e004a52 on fflorent:memchr_change_proposal into 460041a on Geal:master.

@Hywan
Copy link
Contributor

Hywan commented Aug 22, 2017

Using an iterator would be better for safety and readiness for sure. However I am not able to obtain better performances with the iterator API.

It's up to @Geal to decide :-).

@Geal Geal added this to the 4.0 milestone Dec 3, 2017
@Geal Geal modified the milestones: 4.0, 4.1 Feb 3, 2018
@Geal Geal force-pushed the master branch 3 times, most recently from 6c99077 to e7ca818 Compare May 14, 2018 12:06
@Geal Geal force-pushed the master branch 2 times, most recently from fbd7d65 to cfc19f1 Compare May 9, 2019 11:10
@Geal Geal modified the milestones: 4.1, 5.0, 6.0 Apr 6, 2020
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.

None yet

4 participants