comparison of integral Quantities is very slow

[radix]

The short version: comparing integral quantities with Eq can be many orders of magnitude slower than regular comparison. This is magnified for the bigger types, with i64 being about a few thousand times slower than comparing plain i64 values. I assume the same happens for the PartialOrd implementation.

     Running `C:\Users\radix\Projects\pandt\target\release\deps\uom_ops-6068a9413eff2c40.exe --bench`

simple u32              time:   [376.27 ps 383.53 ps 390.64 ps]
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe

simple i64              time:   [366.59 ps 369.57 ps 372.66 ps]
Found 8 outliers among 100 measurements (8.00%)
  2 (2.00%) low mild
  3 (3.00%) high mild
  3 (3.00%) high severe

uom u32                 time:   [808.11 ns 814.27 ns 821.16 ns]
Found 9 outliers among 100 measurements (9.00%)
  3 (3.00%) low mild
  2 (2.00%) high mild
  4 (4.00%) high severe

uom i64                 time:   [1.5856 us 1.5981 us 1.6113 us]
Found 13 outliers among 100 measurements (13.00%)
  1 (1.00%) low severe
  4 (4.00%) low mild
  4 (4.00%) high mild
  4 (4.00%) high severe

Here's the benchmark:

#[macro_use] extern crate criterion;
extern crate uom;
extern crate pandt;

use uom::si::length::centimeter;
use criterion::Criterion;
use pandt::types::{u32units, i64units};

fn simple_eq_u32(n: u32, n2: u32) -> bool { n == n2 }
fn simple_eq_i64(n: u64, n2: u64) -> bool { n == n2 }

fn uom_eq_u32(n: u32units::Length, n2: u32units::Length) -> bool { n == n2 }
fn uom_eq_i64(n: i64units::Length, n2: i64units::Length) -> bool { n == n2 }


fn simple_benchmark(c: &mut Criterion) {
  c.bench_function("simple u32", |b| b.iter(|| simple_eq_u32(100, 200)));
  c.bench_function("simple i64", |b| b.iter(|| simple_eq_i64(100, 200)));
}

fn uom_benchmark(c: &mut Criterion) {
  let u32n = u32units::Length::new::<centimeter>(100);
  let u32n2 = u32units::Length::new::<centimeter>(200);
  let i64n = i64units::Length::new::<centimeter>(100);
  let i64n2 = i64units::Length::new::<centimeter>(200);
  c.bench_function("uom u32", |b| b.iter(|| uom_eq_u32(u32n, u32n2)));
  c.bench_function("uom i64", |b| b.iter(|| uom_eq_i64(i64n, i64n2)));
}

criterion_group!(benches, simple_benchmark, uom_benchmark);
criterion_main!(benches);

By my reading of the code, this stems from the fact that Quantities that aren't from the same ISQ! are compatible with each other -- but at the cost of having to perform a conversion, apparently using num-rational. This is something I would be happy to forego - if uom's Eq implementation had a Rhs = Self, it could just perform a simple comparison. Perhaps a flag to the ISQ! macro could turn off this conversion generation and only support comparison of identical types?

[radix]

So I hacked up my local fork of uom to have Eq and PartialEq that look like this:

        impl<D, Ul, V> $crate::lib::cmp::PartialEq for Quantity<D, Ul, V>
        where
            D: Dimension + ?Sized,
            Ul: Units<V> + ?Sized,
            V: $crate::num::Num + $crate::Conversion<V>,
        {
            #[inline(always)]
            fn eq(&self, other: &Self) -> bool {
                self.value == other.value
            }

            // More methods elided
        }

        impl<D, Ul, V> $crate::lib::cmp::PartialOrd for Quantity<D, Ul, V>
        where
            D: Dimension + ?Sized,
            Ul: Units<V> + ?Sized,
            V: $crate::num::Num + $crate::Conversion<V> + $crate::lib::cmp::PartialOrd,
        {
            #[inline(always)]
            fn partial_cmp(&self, other: &Self) -> Option<$crate::lib::cmp::Ordering>
            {
                self.value.partial_cmp(&other.value)
            }

            // More methods elided
        }

And my runtime (of course) goes back to normal (and no changes were required in my own codebase, since I didn't rely on inter-type comparisons).

Do you think it's reasonable, or out of the question, to have an argument to ISQ! that would generate impls like this instead of using Conversion to support other types?

[iliekturtles]

This is also going to affect other operations like +, -, *, /, ... that work on quantities with different base units and use change_base to handle conversion. Unfortunately all the impls are defined in the system! macro. For the ISQ system the macro is called in uom code so library users wouldn't be able to control which impls are generated.

Using a non-default Cargo feature and duplicating impls similar to what you did above is a possible short-term solution until specialization is stabilized:

#[cfg(all(feature = "unstable", feature = "simple-impl"))]
impl<D, U, V> PartialOrd for Quantity<D, U, V> { ... }

#[cfg(not(feature = "simple-impl"))]
impl<D, Ul, Ur, V> PartialOrd<Quantity<D, Ur, V>> for Quantity<D, Ul, V> { ... }

If you have any other ideas let me know. If you want to put together a PR I'll accept it.

[radix]

Using a non-default Cargo feature and duplicating impls similar to what you did above is a possible short-term solution until specialization is stabilized:

Sadly I think using a cargo feature to make this change would break the rule of "cargo features should only be additive" -- for example see this unfortunate situation involving the nom crate: https://users.rust-lang.org/t/shared-dependencies-and-incompatible-features/15145

I guess I can just keep using my fork until specialization happens but I guess it's going to be a long time before this gets solved for a large audience :(

[iliekturtles]

Good point in regards to features only being additive. What about keeping the current impls for f32 and f64 and creating new impls for the remaining storage types that only work on equal types?

storage_types! {
    types: Float;

    impl<D, Ul, Ur> PartialOrd<Quantity<D, Ur, V>> for Quantity<D, Ul, V> { ... }
}

storage_types! {
    types: PrimInt, BigInt, ...;

    impl<D, U> PartialOrd for Quantity<D, U, V> { ... }
}

[radix]

That would ease my performance problems in uom, but it seems like a really surprising design -- having such different behavior between integers and floats.

I guess at this point these performance problems have caused me to develop a bias towards completely disabling auto-conversion everywhere (it was a surprise to me anyways, since "zero-cost" is in the first sentence of the description of uom). But of course, I'm just a new user of your library; I really don't want to show up and start complaining about how it works! These are just my experiences. All that aside, any way for me to avoid auto-conversion on basic ops would solve my problems.

[iliekturtles]

Thinking about this one more the feature idea could work by making the absence of the feature just have impls for quantities of the same base unit and the addition of the feature would change the impls to work for any base quantity. The feature is now additive but has a performance implication for integer types which can be documented until specialization and const fn land.

[radix]

Oh, yes! And it could be enabled by default, which would make it backwards compatible, I think...? Great idea! I can try implementing this this weekend.

[radix]

Well, backwards compatible for people who don't have default-features = false in their dependency, anyway.

[iliekturtles]

@radix Did you still want to finish this effort or would you mind if I picked up where you left off?

[radix]

Hey @iliekturtles ! sorry I dropped the ball on this.

As I recall, I got stuck in my efforts when I started thinking about how to support explicit conversion when autoconvert is off. I can't really remember how far I got. I believe some unit tests are failing depending on the features that are enabled.

Here's the branch I was working on: https://github.com/radix/uom/commits/optional-autoconvert

[iliekturtles]

I've been busy with other things too. The From trait is probably the right way to allow for explicit conversions between types with different base units (#58 added). I took your commit and fixed up a few items if you want to review: da993c0. I'll probably try to do a final review this weekend and then merge into master.

[radix]

@iliekturtles I'll take a look now, thanks!

[radix]

@iliekturtles it looks good to me. as I recall I was also thinking about how to implement From and also worried about specialization. My app can get by without From by just converting manually for now, though.

[iliekturtles]

Thanks for taking a look. I plan to do my final review tomorrow and merge into master.

A custom From-style trait may be the short term answer. I didn't put a lot of time into conversions yesterday.