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

perf: remove more redundant scanning #251

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Conversation

Eh2406
Copy link
Member

@Eh2406 Eh2406 commented Aug 22, 2024

Doing the stronger version of #175 because it turns out to be important to cargo crates.

While investigating Eh2406/pubgrub-crates-benchmark#6 I took a shot in the dark and thought it might have to do with this optimization. Indeed this one change brings the time to analyze the Solana crates down from 127.25min to 90.85min a 28% improvement.

Doing the stronger version of pubgrub-rs#175 because it turns out to be important to cargo crates.
@mpizenberg
Copy link
Member

Nice one! Is it not too penalizing for the other cases?

@Eh2406
Copy link
Member Author

Eh2406 commented Aug 22, 2024

2% regression for elm 3% improvement from "large case", so basically all within the noise.

@Eh2406 Eh2406 added this pull request to the merge queue Aug 22, 2024
Merged via the queue into pubgrub-rs:dev with commit 597bf9e Aug 22, 2024
4 checks passed
@Eh2406 Eh2406 deleted the dedup-more branch August 22, 2024 22:16
@konstin
Copy link
Member

konstin commented Aug 23, 2024

I've rebased uv onto the latest commits and the are no changes in our benchmarks.

image

@Eh2406
Copy link
Member Author

Eh2406 commented Aug 23, 2024

That makes perfect sense. This only really makes a difference in certain "pathological" cases. They just happened to be so slow that they are a large percentage of the benchmark suite I've chosen. But I'm glad it didn't regress your use cases.

maciektr pushed a commit to software-mansion-labs/pubgrub that referenced this pull request Nov 4, 2024
Doing the stronger version of pubgrub-rs#175 because it turns out to be important to cargo crates.
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.

3 participants