-
Notifications
You must be signed in to change notification settings - Fork 21
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
ByteSubstring: add an option to used an owned needle (not borrowed) #50
base: main
Are you sure you want to change the base?
Conversation
e9cb819
to
19b5a06
Compare
@shepmaster i would be happy to rebase on current |
Oh, wow, I'm sorry that this totally dropped off my radar! I think the idea has merit, but I think I would suggest a different implementation. Right now, we have
This way, you can choose if you need ownership or just a reference. Does that make sense and seem plausible? |
After musing on it for a while I wonder if we could change |
Hi @shepmaster |
i think you’ve got a few months longer before you need to start apologizing for anything like that 😅 |
19b5a06
to
eb07c4b
Compare
@shepmaster I've updated the PR in accordance with our discussion:
let me know what you think. |
I just realized that I only compiled it on my macos m1. So only the fallback code was tested 🤦♂️ I will fix and update here... |
Ok. Fixed. @shepmaster would really love to see your comments. |
@shepmaster kind ping on that |
@shepmaster kind ping :) |
4c42031
to
1248666
Compare
I did one force-push to tidy up some formatting things, and a second force-push to rebase on the current main branch (which has some soundness fixes). Overall, things seem reasonable, but I'm a bit hesitant about the Have you tried this code locally with a |
This allows to either own (consume) the needle or keep a reference to it.
This allows to either own (consume) the needle or keep a reference to it.
This gains two purposes: 1. Fix ownership issue of trying to own needle `T` by both simd & fallback modules. 2. Simplifies the runtime checks done by the code when running in `maybe` mode.
1248666
to
a420c3f
Compare
Hi @shepmaster Thanks for the review.
Bottom line, I fixed the code a little more (now for all features), benchmarks are running properly - and obviously the new code was forced pushed into the branch. |
I don't understand exactly how, but the benchmarks all show a marked decrease in speed with this branch. That's even including the benchmarks for the standard library versions, which obviously shouldn't change!
|
Yeah, I think that this change will warrant a new semver-incompatible version to be released; that's not a big deal IMO.
Yeah, I read through that before posting my comment (that's what led to the "I believe"). It must have been made up in my head. |
That is to be expected. Benchmarks will usually show differences of +-3% depending on their nature and whatever the benchmarking server is doing at the moment. What I usually did when I was running benchmarks using nightly-rust was to externally run them again and again and look at the avg/max/min of each individual benchmark. If you would like, it is possible to migrate the benchmarks to use criterion, which saves you the hassle of what described above. There is also a nice tool called critcmp which helps in comparing the results of two different criterion executions. |
oh, and criterion also runs with stable rust, which is a bonus... |
sorry, i'm looking again at the stdlib difference, and it is weird. too high of a diff. let me look into it a little more. |
This PR is a suggestion. I have done a minimal implementation, just so we have something to discuss.
The problem I was facing was that I couldn't use a borrowed
ByteSubstring
. So, I've modified jetscii's code to accept aCow
object.Let me know what you think and if there're are any more modifications you would like to see as part of this PR.