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

Add Const<T> impl through 1024 #1111

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

trueb2
Copy link
Contributor

@trueb2 trueb2 commented May 25, 2022

This allows using larger constant size dimensions for a number of interfaces. This is particularly useful in no_std situations where statically sized matrix algebra should be possible but isn't because of the hard limit at 127.

I am not sure if this affects anything other than compile times, so I checked that before and after. Incremental builds take a couple seconds depending on the file touched, but there is a difference on a clean build.

  • Before this change from clean:
    • Debug: 17.52s
    • Release: 15.94s
  • After this change from clean:
    • Debug: 52.69s
    • Release: 45.35s

There are a number of related issues like #1108

@Ralith
Copy link
Collaborator

Ralith commented May 26, 2022

I would very much prefer for the clean build time to not be so massively inflated. If this is absolutely needed for your use case (are you sure you can't use alloc?) then perhaps it could be feature-gated?

@trueb2
Copy link
Contributor Author

trueb2 commented May 26, 2022

I would very much prefer for the clean build time to not be so massively inflated. If this is absolutely needed for your use case (are you sure you can't use alloc?) then perhaps it could be feature-gated?

I agree that clean build time inflation is an issue of consideration but do think it shouldn't prevent using the library in no_std or no alloc situations. Since this library is so powerfully generic and this is common problem that new comers hit, I think there should be a solution that generally works and has a workaround rather than having to dive into the macro call site and GitHub issues.

Personally, I have a couple embedded projects where I have a couple KBs left. I mostly want to use power of two sizes, but the RAM constraints of embedded make it such that I can't always pick a nice round number. For example, various applications may only be able to dedicate something like 10-50KB for the stack usage and the static memory buffers. On top of that dynamic memory allocation is simply unacceptable due to the unpredictable regularity of OOM.

Feature-gating is a problem for me because libraries that use nalgebra as a dependency and interface on the types would need features configured. However, cargo doesn't have a pattern that maintainers follow for saying no_std, no alloc and just feature X enabled in a shared dependency.

If I had the option of specializing Const for Typenum as needed for my use case from the call site module, I would be satisfied. If I had just powers of two and multiples of 25, I would also be satisfied. I would also like if the compiler error was a little more helpful when I try to use Const and Typenum on a Const number that is out of range.

@sebcrozet
Copy link
Member

sebcrozet commented May 26, 2022

Hey! Thank you for the proposal. Clean compile times of nalgebra is already a common subject of complain within the community, so multiplying them by 3 isn’t a good option. This will also increase the size of the .rlib (but I don’t believe that maters much). Feature-gating sounds like the only reasonable solution here.

However, cargo doesn't have a pattern that maintainers follow for saying no_std, no alloc and just feature X enabled in a shared dependency.

I’m not sure I understand the problem with feature-gating here. The user could depend on nalgebra like this:

[dependencies]
nalgebra = { version = "*", default-features = false, features = [ "large-const-generics" ] }

This won’t enable the std or alloc features of nalgebra.

If I had the option of specializing Const for Typenum as needed for my use case from the call site module, I would be satisfied. If I had just powers of two and multiples of 25, I would also be satisfied. I would also like if the compiler error was a little more helpful when I try to use Const and Typenum on a Const number that is out of range.

Unfortunately, we are limited by Rust’s currently minimal support of const-generics here.

@trueb2
Copy link
Contributor Author

trueb2 commented May 26, 2022

Hi @sebcrozet, thanks for helping to maintain this awesome repo!

To clarify my issue with

[dependencies]
nalgebra = { version = "*", default-features = false, features = [ "large-const-generics" ] }

My concern centered around adding a generic feature to a powerful base crate consumed by many crates that will require documentation and feature additions to bubble up through the community.

I did some closer reading on Feature Unification and think adding this behind a feature flag will be fine. I can see how adding the values >= 128 behind a feature 'large-const-generics' and bumping the version throughout the consuming crates could solve the issue.

Features are unique to the package that defines them. Enabling a feature on a package does not enable a feature of the same name on other packages.
When a dependency is used by multiple packages, Cargo will use the union of all features enabled on that dependency when building it. This helps ensure that only a single copy of the dependency is used. See the features section of the resolver documentation for more details.

If the compile times are such a concern, would you consider adding more of the const-generic specialization behind feature flags? For example, 0 through 16 always compiled in, 17 through 128 behind 'const-generics-128', 128 through 1024 behind 'const-generics-1024'.

Build from clean with just 0 through 16, debug was 13.26s and release was 11.81.

Less than 16 won't compile due to specializations

// Implement for vectors of dimension 1 .. 16.
impl_from_into_asref_1D!(
    // Row vectors.
    (U1, U1 ) => 1;  (U1, U2 ) => 2;  (U1, U3 ) => 3;  (U1, U4 ) => 4;
    (U1, U5 ) => 5;  (U1, U6 ) => 6;  (U1, U7 ) => 7;  (U1, U8 ) => 8;
    (U1, U9 ) => 9;  (U1, U10) => 10; (U1, U11) => 11; (U1, U12) => 12;
    (U1, U13) => 13; (U1, U14) => 14; (U1, U15) => 15; (U1, U16) => 16;

    // Column vectors.
                     (U2 , U1) => 2;  (U3 , U1) => 3;  (U4 , U1) => 4;
    (U5 , U1) => 5;  (U6 , U1) => 6;  (U7 , U1) => 7;  (U8 , U1) => 8;
    (U9 , U1) => 9;  (U10, U1) => 10; (U11, U1) => 11; (U12, U1) => 12;
    (U13, U1) => 13; (U14, U1) => 14; (U15, U1) => 15; (U16, U1) => 16;
);

@Ralith
Copy link
Collaborator

Ralith commented May 27, 2022

will require documentation and feature additions to bubble up through the community.

Yeah, fortunately not necessary.

this is common problem that new comers hit

Is it? I'm not sure what proportion of users are doing high-dimensional math to begin with, and I expect the overwhelming majority of those are happily using dynamic sizes.

Build from clean with just 0 through 16, debug was 13.26s and release was 11.81.

Ooh, about 25% savings, not bad! Might be nice to get in the next breaking release. We could even go lower by gating some of the specializations; there's certainly a segment of users that don't need static sizes for anything > 4, if that, but I expect the returns start diminishing at some point.

@trueb2
Copy link
Contributor Author

trueb2 commented May 27, 2022

I checked trimming down some of those vector and matrix specializations on my machine. The const R values are the minimum before some of the other stuff that uses const like DualQuaternion fails to build.

// Implement for vectors of dimension 1 .. 4.
impl_from_into_asref_1D!(
    // Row vectors.
    (U1, U1 ) => 1;  (U1, U2 ) => 2;  (U1, U3 ) => 3;  (U1, U4 ) => 4;

    // Column vectors.
    (U2 , U1) => 2;  (U3 , U1) => 3;  (U4 , U1) => 4;
);

// Implement for matrices with shape 2x2 .. 4x4.
impl_from_into_asref_borrow_2D!(
    (U2, U2) => (2, 2); (U2, U3) => (2, 3); (U2, U4) => (2, 4); 
    (U3, U2) => (3, 2); (U3, U3) => (3, 3); (U3, U4) => (3, 4);
    (U4, U2) => (4, 2); (U4, U3) => (4, 3); (U4, U4) => (4, 4);
);

from_to_typenum!(
    /*U1,1;*/ U2, 2; U3, 3; U4, 4; U5, 5; U6, 6; U8, 8
);

There wasn't any further change in build time on my machine (13.1s and 11.9s). Since it seems that the majority of the build time changes are just due to the const to typenum invocation, I suppose that I should update the PR

  • without changes to the vector and matrix specializations
  • with 0 through 16 always enabled
  • with 'const-generics-128' feature flag
  • with 'const-generics-512' feature flag
  • with 'const-generics-1024' feature flag

@Ralith
Copy link
Collaborator

Ralith commented May 27, 2022

A change that move existing code behind a feature gate is breaking, so it might be useful to split that up from the one that adds new code, if you/@sebcrozet want to get the extended sizes in a patch release soon.

@trueb2
Copy link
Contributor Author

trueb2 commented May 31, 2022

I should have commented earlier, but the previous commit moved just the new specializations through 1024 behind a feature flag. Therefore, this PR is not a breaking change.

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.

None yet

3 participants