Optimize mat_entry_ptr methods #1871

fingolfin · 2024-09-30T08:56:55Z

No description provided.

codecov · 2024-09-30T09:03:00Z

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.32%. Comparing base (8b486cc) to head (2af5415).

Files with missing lines	Patch %	Lines
src/flint/nmod_mat.jl	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1871      +/-   ##
==========================================
- Coverage   87.33%   87.32%   -0.01%     
==========================================
  Files          97       97              
  Lines       35858    35859       +1     
==========================================
- Hits        31316    31314       -2     
- Misses       4542     4545       +3

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

lgoettgens · 2024-09-30T10:15:36Z

src/flint/FlintTypes.jl

@@ -6204,10 +6204,10 @@ end
 const fqPolyRepMatrixSpace = AbstractAlgebra.Generic.MatSpace{fqPolyRepFieldElem}

 mutable struct fqPolyRepMatrix <: MatElem{fqPolyRepFieldElem}
-  entries::Ptr{Nothing}
+  entries::Ptr{fqPolyRepFieldElem}


I think this is not 100% true, since otherwise I would expect the following to work (before this PR, with htis PR the Ptr conversion becomes unnecessary):

julia> F = Native.GF(2,2); julia> a = matrix(F, 2, 2, [1,2,3,4]); julia> unsafe_load(Ptr{fqPolyRepFieldElem}(a.entries)) [1054580] signal (11.1): Segmentation fault

It definitely is a lie, just like with many other Ptr{ZZRingElem} etc. etc. in our code base -- our Julia types are "supersets" of the FLINT types.

BTW you can get the same crash in master via unsafe_load(Nemo.mat_entry_ptr(a, 1, 1)). I simply chose the signatures here to make the implementation of mat_entry_ptr as simple as possible for each type.

We could of course also resolve this by adding a separate struct fq_nmod_t (and fmpz_t etc. etc.) which faithfully replicate the FLINT structs, nothing more and nothing less. Then e.g. fqPolyRepFieldElem could start with a member x:: fq_nmod_t.

That way, we could resolve all the lies everywhere. But that would be a major piece of work for relatively limited gain (if I were to start Nemo today, that's more or less what I'd do anyway, but as it is now...).

Anyway: I am happy to discuss this further, but I think for the purpose of this PR, it doesn't really make anything worse, as nothing in Nemo.jl ever accesses .entries fields, and the access to .rows is limited to very, very few places. And to me at least it was helpful to get a better idea of what each entries and rows points to. But I am also ok with changing them back to Nothing and adjust the mat_entry_ptr to insert suitable reinterpret calls.

Thanks for the explanation. I am fine with merging this. Could you open an issue with basically what you explained here, so that we keep it somewhere? (even though that's nothing to be changed in the near future (unless someone has way too much time))

lgoettgens

CI is happy, I don't really understand all of the formulas, but trust you with this

fingolfin · 2024-09-30T13:57:14Z

src/flint/fmpq_mat.jl

-@inline mat_entry_ptr(A::QQMatrix, i::Int, j::Int) = 
-ccall((:fmpq_mat_entry, libflint), 
-      Ptr{QQFieldElem}, (Ref{QQMatrix}, Int, Int), A, i-1, j-1)
+mat_entry_ptr(A::QQMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*2*sizeof(Ptr)


Suggested change

mat_entry_ptr(A::QQMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*2*sizeof(Ptr)

mat_entry_ptr(A::QQMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(QQFieldElem)

fingolfin · 2024-09-30T13:58:49Z

src/flint/fmpz_mat.jl

@@ -2107,4 +2107,4 @@ end
 #
 ################################################################################

-@inline mat_entry_ptr(A::ZZMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(UInt)
+mat_entry_ptr(A::ZZMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)


Suggested change

mat_entry_ptr(A::ZZMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)

mat_entry_ptr(A::ZZMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(ZZRingElem)

fingolfin · 2024-09-30T13:59:25Z

src/flint/fmpz_mod_mat.jl

-@inline mat_entry_ptr(A::ZZModMatrix, i::Int, j::Int) =
-ccall((:fmpz_mod_mat_entry, libflint), Ptr{ZZRingElem},
-      (Ref{ZZModMatrix}, Int, Int), A, i - 1, j - 1)
+mat_entry_ptr(A::ZZModMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)


Suggested change

mat_entry_ptr(A::ZZModMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)

mat_entry_ptr(A::ZZModMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(ZZRingElem)

fingolfin · 2024-09-30T14:07:21Z

src/flint/fq_mat.jl

-@inline mat_entry_ptr(A::FqPolyRepMatrix, i::Int, j::Int) =
-ccall((:fq_mat_entry, libflint), Ptr{FqPolyRepFieldElem},
-      (Ref{FqPolyRepMatrix}, Int, Int), A, i - 1, j - 1)
+mat_entry_ptr(A::FqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+2*sizeof(Int))


This is a bit confusing, perhaps I should add a comment

Suggested change

mat_entry_ptr(A::FqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+2*sizeof(Int))

# each matrix entry consists of

# coeffs :: Ptr{Nothing}

# alloc :: Int

# length :: Int

# The `parent` member of struct FqPolyRepFieldElem is not replicated in each

# struct member, so we cannot use `sizeof(FqPolyRepFieldElem)`.

mat_entry_ptr(A::FqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+2*sizeof(Int))

fingolfin · 2024-09-30T14:10:59Z

src/flint/fq_nmod_mat.jl

-@inline mat_entry_ptr(A::fqPolyRepMatrix, i::Int, j::Int) =
-ccall((:fq_nmod_mat_entry, libflint), Ptr{fqPolyRepFieldElem},
-      (Ref{fqPolyRepMatrix}, Int, Int), A, i - 1, j - 1)
+mat_entry_ptr(A::fqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+5*sizeof(Int))


Suggested change

mat_entry_ptr(A::fqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+5*sizeof(Int))

# each matrix entry consists of

# coeffs :: Ptr{Nothing}

# alloc :: Int

# length :: Int

# n :: Int

# ninv :: Int

# norm :: Int

# The `parent` member of struct fqPolyRepFieldElem is not replicated in each

# struct member, so we cannot simply use `sizeof(fqPolyRepFieldElem)`.

mat_entry_ptr(A::fqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+5*sizeof(Int))

fingolfin · 2024-09-30T14:11:37Z

src/flint/gfp_fmpz_mat.jl

-@inline mat_entry_ptr(A::FpMatrix, i::Int, j::Int) =
-ccall((:fmpz_mod_mat_entry, libflint), Ptr{ZZRingElem},
-      (Ref{FpMatrix}, Int, Int), A, i - 1, j - 1)
+mat_entry_ptr(A::FpMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)


Suggested change

mat_entry_ptr(A::FpMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)

mat_entry_ptr(A::FpMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(ZZRingElem)

lgoettgens · 2024-09-30T14:16:38Z

These explanations really help! Thanks!

Optimize mat_entry_ptr methods

2af5415

lgoettgens reviewed Sep 30, 2024

View reviewed changes

lgoettgens approved these changes Sep 30, 2024

View reviewed changes

fingolfin commented Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize mat_entry_ptr methods #1871

Optimize mat_entry_ptr methods #1871

fingolfin commented Sep 30, 2024

codecov bot commented Sep 30, 2024 •

edited

Loading

lgoettgens Sep 30, 2024

fingolfin Sep 30, 2024

lgoettgens Sep 30, 2024

lgoettgens left a comment

fingolfin Sep 30, 2024

fingolfin Sep 30, 2024

fingolfin Sep 30, 2024

fingolfin Sep 30, 2024 •

edited

Loading

fingolfin Sep 30, 2024

fingolfin Sep 30, 2024

lgoettgens commented Sep 30, 2024

	mat_entry_ptr(A::QQMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)2sizeof(Ptr)
	mat_entry_ptr(A::QQMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(QQFieldElem)

	mat_entry_ptr(A::ZZMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)
	mat_entry_ptr(A::ZZMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(ZZRingElem)

	mat_entry_ptr(A::ZZModMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)
	mat_entry_ptr(A::ZZModMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(ZZRingElem)

-mat_entry_ptr(A::FqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+2*sizeof(Int))
+# each matrix entry consists of
+#   coeffs :: Ptr{Nothing}
+#   alloc :: Int
+#   length :: Int
+# The `parent` member of struct FqPolyRepFieldElem is not replicated in each
+# struct member, so we cannot use `sizeof(FqPolyRepFieldElem)`.
+mat_entry_ptr(A::FqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+2*sizeof(Int))

	mat_entry_ptr(A::FpMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)
	mat_entry_ptr(A::FpMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(ZZRingElem)

Optimize mat_entry_ptr methods #1871

Are you sure you want to change the base?

Optimize mat_entry_ptr methods #1871

Conversation

fingolfin commented Sep 30, 2024

codecov bot commented Sep 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgoettgens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgoettgens commented Sep 30, 2024

codecov bot commented Sep 30, 2024 •

edited

Loading

fingolfin Sep 30, 2024 •

edited

Loading