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

Faster Ord when Eq #328

Merged
merged 1 commit into from
Dec 12, 2024
Merged

Faster Ord when Eq #328

merged 1 commit into from
Dec 12, 2024

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Dec 12, 2024

I have a tool for benchmarking Cargoes resolver and a new PubGrub based resolver. Both resolvers spend a lot of time checking whether two Versions are equal using Ord. For example, .contains on a BTreeMap<Versions, .... @arielb1 pointed out that adding a fast path makes a big difference in the runtime of the resolvers. On master the program reports PubGrub CPU time: 86.97min, Cargo CPU time: 190.67min, with this PR PubGrub CPU time: 71.33min, Cargo CPU time: 167.92min. (For posterity the command is cargo r -r -- -t 100 running commit 9dc991f94c45688a6d2d90d29532c5d96fd064b3) there is about +/- 3 min of run-to-run variability.

In most cases Prerelease and BuildMetadata are empty. Prerelease already did not do much work for the empty == empty case, most of the time difference therefore comes from BuildMetadata.

I also considered a simpler

if self.identifier.is_empty() && rhs.identifier.is_empty() {
    return Ordering::Equal;
}

Which is not as big an improvement (PubGrub CPU time: 75.85min).

Other variations that get approximately the same result, at least within the margin of error are:

if self.identifier == rhs.identifier {
    return Ordering::Equal;
}

or

if self.identifier.as_str().as_ptr() == rhs.identifier.as_str().as_ptr() {
    return Ordering::Equal;
}

What are your thoughts about this optimization? Which variation would you prefer? Is there some testing you would like added?

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

This looks great to me. Thank you!

@dtolnay dtolnay merged commit 238757d into dtolnay:master Dec 12, 2024
19 checks passed
@dtolnay
Copy link
Owner

dtolnay commented Dec 12, 2024

Published in 1.0.24.

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.

2 participants