RFC: redesign of the flint wrapper

[thofma]

Motivation

The FLINT wrapper is inefficient when accessing elements of matrices or polynomials (or anything else nested). The problem is that to acesss M[i, j] we first create a new fmpz and then fmpz_set the elements. As a consequence, we now have a lot of mat_entry_ptr functions, which are very (very) unsafe and plenty of ZZRingElemOrPtr occurences.

Possible solution

I had many discussions with @fieker and Dan over the last few years and here is one altenative design which is distilled from these discussions:

struct ZZRingElemDR
  ptr::Ptr{ZZRingElem}
  data #= can be anything where ptr points to (fmpz, fmpz_mat, fmpz_poly, ...) =#
end

("DR" stands for "done right", a tribute to Dan.)

The idea is to have an (immutable) layer on top of the raw fmpz, which holds a pointer to an fmpz together with "data". The "data" part is only there to keep the memory alive for the sake of the GC. It can be a ZZRingElem, or a ZZMatrix or a ZZPolyRingElem, and so on.

Together with some Base.cconvert, it is relatively easy to make this fly, see the diff:

julia> a = Nemo.ZZRingElemDR(ZZ(2))
2

julia> -a
-2

julia> a = Nemo.ZZRingElemDR(ZZ[1 2; 1 1], 1, 2)
2

julia> is_unit(a)
false

julia> Zx, x = ZZ["x"]; f = x^3 + 213*x + 1;

julia> -Nemo.ZZRingElemDR(f, 1)
-213

The important point is that this is allocation free:

julia> M = ZZ[1 2; 3 4];

julia> @ballocated getindex(M, 1, 1)
16

julia> @ballocated Nemo.ZZRingElemDR(M, 1, 2)
0

As @lgoettgens pointed out, for more complicated things the top layer "DR" also needs the parent in case the bottom layer goes parent-less.

struct fqPolyRepFieldDR
  ptr::Ptr{fqPolyRepFieldElem}
  data
  parent::fqPolyRepField

What we gain

It will be easy to write generically fast code:

a = M[i, j]
mul!(b, a)

would be fast for any type (it is slow for FLINT types at the moment).

Drawback

This increases the size of FLINT types. We add 16 bytes per object. For small fmpz, this makes them 2x(?) times larger. (At the moment they should be 8 bytes for the pointer/small int + 8 bytes because mutable).

How much do we have to rewrte? Is it breaking?

It should be relatively easy. One would rename the current ZZRingElem to something else, like ZZRingElemRaw or fmpz. Then rename ZZRingElemDR to ZZRingElem. With the Base.cconvert trick, even the ccalls in Hecke/Oscar would be "easy" to adjust.

It "should" not be too breaking.

Other advantage

We might even use this to have the raw flint layer (that can be constructed "automatically" from the headers as @lgoettgens demonstrated) and put all other things in the top layer (like parent objects etc).

CC: @fieker @fingolfin @lgoettgens

[lgoettgens]

How would this work for Nemo types that have a different memory layout than the FLINT type? eg a ZZPolyRingElem having the parent as a field?
In these cases, a ptr to a flint matrix of such elements is not sufficient to recover the parent. And using the data field would lead to dynamic dispatch

Edit: reading is hard. You already answered the question in the Other advantage section

[codecov]

Codecov Report

Attention: Patch coverage is 9.09091% with 10 lines in your changes missing coverage. Please review.

Project coverage is 88.03%. Comparing base (2363f8d) to head (9d86e4b).

Files with missing lines	Patch %	Lines
src/flint/fmpz.jl	12.50%	7 Missing ⚠️
src/flint/FlintTypes.jl	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2058      +/-   ##
==========================================
- Coverage   88.07%   88.03%   -0.05%     
==========================================
  Files         100      100              
  Lines       36903    36913      +10     
==========================================
- Hits        32503    32497       -6     
- Misses       4400     4416      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[thofma]

Good point! What to do with the parent depends a bit on how the bottom layer looks like: Whether it is parent-less or parent-full.

If we do the cheap version of just putting a new layer on top of what we now have + renaming types and adding methods (but not changing the current things), the top layer would not need the parent information, since one can always reconstruct it from the data.

If we rewrite/use the raw layer with parent-less raw objects, we need to put the parent in the top layer.

[thofma]

We (@fingolfin @fieker and myself) decided to try this with ZZRingElem and see how much fun this is to adjust the code downstream. I will do this.

[lgoettgens]

Another interesting part would be implementing copy and/or deepcopy. Should this copy the data field as well? Or would it be better to copy the one fmpz out of the object and then use that as the data of the copy?

Maybe @thofma can play around with the options a bit when trying to make this work

[thofma]

I'll keep copy and deepcopy the way they are. Where the pointer points to (fmpz, fmpz_mat etc) is just an implementation detail.

[thofma]

I adjusted a lot of code, until I realized that this approach has a serious drawback. One cannot actually swap matrix elements!

julia> M = ZZ[1 2]
[1   2]

julia> M[1, 1], M[1, 2] = M[1, 2], M[1, 1];

julia> M
[2   2]

Also

t = M[1, 2]; M[1, 2] = M[1, 1]; M[1, 1] = t

won't work.

I think this is a no-go in terms of replacing ZZRingElem with this new type.

What I now realize is that I have written a GC-friendly version of @view M[i, j]. So unless someone finds a clever way to salvage this, one way to make of use of this would be to

• introduce the new type ZZRingElemView,
• make @view M[i, j] return a ZZRingElemView,
• use the @view M[i, j] syntax instead of mat_entry_ptr.
• also use @view coeff(f, i) for ZZPolyRingElem.