Skip to content

Optimized Ord::cmp function#155

Open
mikem8891 wants to merge 2 commits into
rust-num:mainfrom
mikem8891:ord-cmp
Open

Optimized Ord::cmp function#155
mikem8891 wants to merge 2 commits into
rust-num:mainfrom
mikem8891:ord-cmp

Conversation

@mikem8891

Copy link
Copy Markdown

This implementation of the Ord::cmp function does several early comparisons to try to determine ordering before resulting to div and mod operations. This allows about half of unsigned Ratio<T> pairs and 3/4 of signed Ratio<T> pairs to be determined using simple comparisons (based on the entire input space, individual results may vary). If simple comparisons don't determine the ordering, then the function enters a tight loop to converge on a solution as quickly as possible.

Note, #121 would be faster, but would still benefit from this pull request if checked_mul returns None.

I confirmed this fixes #140 . This solution is iterative instead of recursive.

This should also fixes #146 . I phoned this in because I think negative denominators should not be adversely affecting performance or complexity compared to normal fractions.

For context, here are some benchmark results

new_test_cmp_overflow   time:   [1.8502 µs 1.8865 µs 1.9358 µs]
Found 17 outliers among 100 measurements (17.00%)
  3 (3.00%) high mild
  14 (14.00%) high severe

old_test_cmp_overflow   time:   [2.3391 µs 2.3740 µs 2.4193 µs]
Found 6 outliers among 100 measurements (6.00%)
  5 (5.00%) high mild
  1 (1.00%) high severe

I can make this benchmark available to you if needed.

This is a repost of #147 because I accidentally closed it after the crate was refactored into modules.

@cuviper

cuviper commented Jul 8, 2026

Copy link
Copy Markdown
Member

I have in mind a way to make the current code iterative without changing the overall approach as much. I would at least like to compare that to yours, so if you could share your benchmark, that would be great.

@mikem8891

Copy link
Copy Markdown
Author

https://github.com/mikem8891/num-bench

cargo run tests #140

cargo bench benchmarks Ord::cmp function in the crate in the num-rational folder to the num-rational crate in crates.io, per the Cargo.toml snippet below.

[dependencies]
old-num-rational = {version = "0.4.2", package = "num-rational"}
new-num-rational = {path = "num-rational", package = "num-rational"}

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.

Ord:cmp does not properly handle Ratio<T> with negative denominators Stackoverflow with PartialEq and PartialOrd

2 participants