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

Fully optimize cargo release builds and add debuggable release profile #2184

Closed
wants to merge 3 commits into from
Closed

Conversation

SUPERCILEX
Copy link

@SUPERCILEX
Copy link
Author

Ah, looks like the tests are using rust 1.52 but this needs 1.57: rust-lang/cargo#9943

@SUPERCILEX
Copy link
Author

@BurntSushi is there any interest in this? I'm wondering if it's worth bumping the MSRV.

@BurntSushi
Copy link
Owner

BurntSushi commented Jun 20, 2022

I'm totally fine to bump the MSRV, up to and including the latest Rust stable release.

In general, I don't really look at PRs like this unfortunately until I actually do a ripgrep release. At which point, I'll decide whether to bring it in by actually trying it out and playing around with it. If changes need to be made, I'll just do them by squashing them into your commit. If I didn't do that, then I'd be context switching all of the time and would get nothing done. Since I became a Dad, my release cycles have become longer.

So this PR might sit for some time. But I might be interested. Hard to say without actually giving it a whirl and seeing what it's actually like.

@SUPERCILEX
Copy link
Author

Ok, sounds good! I bumped the MSRV so this should be ready to merge if you decide you like it.

BTW, dunno if people have mentioned this before, but a big reason for supporting cargo installation is that you can use rustflags = ["-C", "target-cpu=native"] which will presumably get you slightly higher performance.

@BurntSushi
Copy link
Owner

BTW, dunno if people have mentioned this before, but a big reason for supporting cargo installation is that you can use rustflags = ["-C", "target-cpu=native"] which will presumably get you slightly higher performance.

Yeah I'm aware of that. I personally think cargo install is just more about convenience. Setting target-cpu=native is likely to help a little bit in certain workloads, but ripgrep already uses SIMD in its hot paths even when target-cpu is not set at all. I don't even bother compiling with target-cpu=native when benchmarking ripgrep. (Partially because I've never really seen it make much of a difference and partially because it's an uncommon build configuration.)

@SUPERCILEX SUPERCILEX closed this by deleting the head repository Feb 2, 2023
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

2 participants