-
Notifications
You must be signed in to change notification settings - Fork 800
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
FindSubstring<&[u8]>: make use of memchr::memmem
#1375
base: main
Are you sure you want to change the base?
Conversation
a33987b
to
129ce0a
Compare
Note: I just force-pushed to retry CI, I could not rerun as "[the] workflow file may be broken." |
129ce0a
to
3de6704
Compare
At this point I'm wondering if using memchr is still the right approach, considering that a lot of parsers looking for a specific token or substring work on very small inputs. Maybe we should compare with a naive implementation |
My findings are
winnow took the approach of putting Recently, I've been working with Byron on porting gitoxide from nom to winnow (Byron/gitoxide#956). Within gitoxide, the use of |
Important to note that for gitoxide,
So its a mix of single-character (the |
substring search and single character matching could work better as different combinators, they are not meant for the same task, and usually don't happen on the same amount of data. Thank you for taking the time to measure this on a larger codebase |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1375 +/- ##
=======================================
Coverage 61.47% 61.47%
=======================================
Files 24 24
Lines 2951 2936 -15
=======================================
- Hits 1814 1805 -9
+ Misses 1137 1131 -6 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #1375 will not alter performanceComparing Summary
|
I guess that #565 could be closed with this one.
memchr
MSRV is rust 1.41.1 so it should be okay.FTR I did run criterion before and after the change