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

Use entries + stride instead of entries + array of row pointers in matrix types #2162

Merged
merged 25 commits into from
Jan 21, 2025

Conversation

fredrik-johansson
Copy link
Collaborator

Per #2064.

In general, this should improve performance by making window matrices and temporary matrices cheaper to create (0 and 1 mallocs instead of 1 and 2 respectively). Also opens up for future optimizations where we can exploit the regular offset between rows.

I haven't benchmarked this carefully to make absolutely sure there are no major performance regressions, but here at least is the time for nmod_mat_solve to solve Ax=b over a 60-bit prime field:

n           OLD           NEW
10          2.02e-06      2.03e-06
20          6.87e-06      5.99e-06
40          3.65e-05      2.62e-05
80          0.000187      0.000151
160         0.00109       0.000946
320         0.00693       0.0064
640         0.0469        0.0447
1280        0.333         0.322
2560        2.289         2.271
5120        15.956        15.876

Breaks compatibility with user code that accesses mat->rows directly. @albinahlback Nemo needs to be fixed as it uses matrix internals in a few places.

An important semantic difference is that row swaps applied to a window matrix now update the contents in the original matrix. In general, this should make life easier.

We did previously rely on being able to create permutation window matrices in can_solve/solve methods. These have been replaced with full temporary matrices, so memory usage may be higher in some cases. There could be workarounds.

I didn't bother changing mpfr_mat, mpf_mat, d_mat and bool_mat in this PR since those types are for internal use. We may want to retain a version of d_mat with the rows pointer anyway since it is used in LLL which does a lot of row swaps (needs profiling).

I found a lot of dirt under the carpet while working on this, which I will probably put in a separate issue.

@fredrik-johansson
Copy link
Collaborator Author

@vneiger might want to review this

@fredrik-johansson
Copy link
Collaborator Author

I don't think there are any major drawbacks, so I will merge as is; followup tasks already noted in #2165.

@fredrik-johansson fredrik-johansson merged commit 25251b8 into flintlib:main Jan 21, 2025
11 of 12 checks passed
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.

1 participant