RFC: Change ComplexField and related traits to improve generic programming
- Dominant language
- Rust
- Stars
- 4.8k
- Forks
- 565
- PR merge metrics
- No merged PRs in 30d
Description
This proposal suggests a breaking change that may impact many users of `nalgebra` in various ways, although I hope the impact will be minimal for the vast majority of users. I am looking for comments from the community.
# Motivation
In `nalgebra` `0.29` if I'm not mistaken, `Copy` was removed from `RealField`. This is great because it opens up using `nalgebra` with non-copy types like arbitrary precision or similar. However, it also led to some severe regressions in the ergonomics of writing generic code. For example, consider the following code that would work in `nalgebra` prior to `0.29`:
```rust
pub fn rotation_matrix(theta: T) -> Matrix2 {
matrix![ theta.cos(), -theta.sin();
theta.sin(), theta.cos() ]
}
```
This code no longer compiles, and would have to be replaced by the following code:
```rust
pub fn rotation_matrix(theta: T) -> Matrix2 {
matrix![ theta.clone().cos(), -theta.clone().sin();
theta.clone().sin(), theta.clone().cos() ]
}
```
The spurious cloning feels unnecessary, hurts readability and might even pessimize performance, especially for non-copy types where clones might not be optimized out by the compiler, such as arbitrary precision types.
# Proposed solution
It would be great to be able to write the code in the first example again. To understand why the clones are necessary, let's take a look at a trimmed down version of the `ComplexField` trait:
```rust
pub trait ComplexField : ... {
// ...
fn cos(self) -> Self;
fn sqrt(self) -> Self;
}
```
Each method takes `self`, which means that if `Self` is not `Copy`, we *must* clone in order to call any of these methods. I believe the reason this trait is designed in this way is that it mirrors the `cos`, `sqrt` etc. methods on `f64` and `f32`. The reason that these methods on `f32`/`f64` take `self` by value is that for primitive types, it tends to generate better assembly than references, though the differences may be negligible or non-existent out in optimized builds where the compiler is able to elide redundant stores/loads.
The obvious solution for fixing our generic predicament is naturally to change these methods to take `self` by reference, e.g.
```rust
pub trait ComplexField : ... {
// ...
fn cos(&self) -> Self;
fn sqrt(&self) -> Self;
}
```
This would fix our initial generic code woes, and would be far more efficient for arbitrary precision types on top.
The same applies to all similar traits, including of course `RealField`.
There are a few potential drawbacks:
- It is a breaking change.
- Performance *may* suffer in some cases.
## Analysis: Breaking changes
I don't think the breaking changes will be too severe, especially for one particular reason: These methods largely already exist on `f32` and `f64`, so that when users are *not writing generic code*, they will anyway be calling the built-in methods directly on the primitive types.
If they're writing generic code, they'll already be writing e.g. `x.cos()` taking `x` by value. This would still compile if the method instead takes `x` by reference.
The change would break the code of users who are writing e.g. `T::cos(x)`, in which case they'd have to update to `T::cos(&x)`, though in many cases they might be writing `T::cos(x.clone())`, in which case I bet the change is welcome.
I welcome comments from users as to whether this change would be detrimental to them. It is a trade-off: we should not take breaking changes of this scale lightly, so we must carefully weigh the cost and benefits.
## Analysis: Performance *may* suffer
Some initial tests on [rust.godbolt.org](https://rust.godbolt.org) suggests that in optimized builds the reference is often elided, if implemented inline. For example, consider this:
```rust
pub trait ComplexField {
fn sqrt(&self) -> Self;
}
impl ComplexField for f64 {
#[inline]
fn sqrt(&self) -> Self {
f64::sqrt(*self)
}
}
pub fn test_sqrt(x: f64) -> f64 {
::sqrt(&x)
}
```
According to [Compiler Explorer](https://rust.godbolt.org/z/WWsEG6xaq), `test_sqrt` pays no penalty for the reference. However, if `#[inline]` is removed, there may be an additional "move" instruction generated, especially in cross-crate usage although this cannot be directly observed on Godbolt.
# Summary
As someone who writes a *lot* of generic code, the number of redundant clones has a tendency to reach ridiculous heights at times, and I'd sorely like to improve this. I've outlined a solution and two potential drawbacks. I'd like to solicit feedback from other users of `nalgebra`, and I think it's clear that we need some performance benchmarks to try to determine if real-world performance suffers from the proposed changes. Besides running `nalgebra`'s own benchmarks, are there other benchmarks that can be used to try to get a good picture of real-world impact?
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.