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

Remove copy requirement from Scalar trait #491

Closed
wants to merge 3 commits into from
Closed

Remove copy requirement from Scalar trait #491

wants to merge 3 commits into from

Conversation

StevenDoesStuffs
Copy link

@StevenDoesStuffs StevenDoesStuffs commented Dec 4, 2018

Fixes #282.
This is the first of two parts, if you accept the PR, I'll work on accepting references for functions/methods that have a scalar as a parameter.

There are two macros I defined, c, and o. running c!(x) does x.clone(), and o!(x) does x.to_owned(). These make the source code quite a bit more clean than writing clone and to_owned out every time.

@jswrenn jswrenn added enhancement breaking change Fixing this issue, or merging this pull-request is likely to require breaking changes labels Dec 4, 2018
@sebcrozet
Copy link
Member

Thank you for to addressing this! Here are some remarks:

  1. Could you check the result of a cargo bench before and after your changes? I'm wondering if this has any impact at all on performances.
  2. Same question but in debug mode. nalgebra is already quite slow when it is compiled in debug mode. Does having .clone() everywhere worsen things?
  3. What about the code constructing new matrices using ::new_uninitialized()? They will be safe for Copy types, but will not be for types that are only Clone. There are a lot of those, see for example in the base/ops.rs file.

I'll work on accepting references for functions/methods that have a scalar as a parameter.

Do you mean all the existing methods taking a N will take a &N instead? This will not be an acceptable change since this will break almost all codebases using nalgebra, and will make using those function much less ergonomic (because of the need to take the reference).

@jswrenn
Copy link
Contributor

jswrenn commented Dec 4, 2018

To say a little more about the performance implications: Copy and Clone are not semantically equivalent. A Copy is always a bitwise copy, not an implicit call to .clone(). As far as I can tell, Rust does not inline these clone calls, even at opt-level=1. Here's a basic example (godbolt):

This:

use std::ops::Add;

pub fn add<N>(a: &N, b: &N) -> <N as Add<N>>::Output
where
    N: Add + Clone
{
    a.clone() + b.clone()
}

pub fn test(a: i32, b: i32) -> i32 {
    add(&a, &b)
}

...compiles to this:

<i32 as core::ops::arith::Add>::add:
  lea eax, [rdi + rsi]
  ret

core::clone::impls::<impl core::clone::Clone for i32>::clone:
  mov eax, dword ptr [rdi]
  ret

example::add:
  push rbp
  push rbx
  push rax
  mov rbx, rsi
  call core::clone::impls::<impl core::clone::Clone for i32>::clone
  mov ebp, eax
  mov rdi, rbx
  call core::clone::impls::<impl core::clone::Clone for i32>::clone
  mov edi, ebp
  mov esi, eax
  add rsp, 8
  pop rbx
  pop rbp
  jmp <i32 as core::ops::arith::Add>::add

example::test:
  push rax
  mov dword ptr [rsp], edi
  mov dword ptr [rsp + 4], esi
  mov rdi, rsp
  lea rsi, [rsp + 4]
  call qword ptr [rip + example::add@GOTPCREL]
  pop rcx
  ret

At the very least, this seems like it may significantly increase the number of emitted instructions.

@StevenDoesStuffs
Copy link
Author

StevenDoesStuffs commented Dec 5, 2018

So first, there will be no breaking change, the methods that currently exist will stay. I'll probably just create new methods with different names.

  1. Benchmarks: https://pastebin.com/gKmM2UEx (non copy scalars), https://pastebin.com/MjfZ1Kx5 (Copy scalars).
    It does appear that there is some performance loss, but maybe with the use of macros we can have the copy scalars not call clone at all which would also fix the issue with the increased amount of instructions. EDIT: running into issues with macro hygiene, and function overloading is disallowed too, the combination of the two makes it impossible. If it were possible to force the clone to inline when we call it then we could fix the instructions issue, but as far as I'm aware rust doesn't allow you to do that.

  2. Not really sure how this impacts compile time, is there a reliable way to benchmark it?

  3. I'll look into that.

And of course I'll fix glm and lapack.
Also, at when opt-level does rust start to do inlining?
EDIT: No clones here: https://rust.godbolt.org/z/QH43j4 at opt-level=2

@jswrenn
Copy link
Contributor

jswrenn commented Dec 5, 2018

Also, at when opt-level does rust start to do inclining?

Cargo's debug/development profile uses opt-level=0, the release profile uses opt-level=3.

@StevenDoesStuffs
Copy link
Author

While working on this PR, I found that there is a type called real in alga that is Copy. I've made a PR dimforge/alga#56 which adds a variant of Real called CloneReal, which allows non-copy types to implement it.

@lesurp
Copy link

lesurp commented Jun 14, 2020

Hi,
I'm currently toying with a symbolic calculus library and I was hoping to use nalgebra with a custom scalar type that does not implement copy.
From the looks of it, all issues related to removing Copy constraints are closed/inactive (dimforge/alga#71, dimforge/alga#56, and this one as well). Is anyone interested in this (or working on it)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Fixing this issue, or merging this pull-request is likely to require breaking changes enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants