Make low-level ZZRingElem accessors more powerful #1942
Conversation
... and their code more readable (at least to me).
Also implement `rem(::ZZRingElem, ::Type{UInt})` and use that to
speed up `iseven`, `isodd`, `sign`, `signbit` and `is_squarefree`
for `ZZRingElem`.
When tested in isolation using BenchmarkTools, the new methods
are usually a little bit faster, and never slower,
than the old ones. The main gain however is in code using them:
unlike opaque `ccall`s the compiler can inline these helper and
possibly perform further optimizations.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1942 +/- ##
==========================================
- Coverage 87.90% 87.89% -0.02%
==========================================
Files 99 99
Lines 36366 36361 -5
==========================================
- Hits 31968 31958 -10
- Misses 4398 4403 +5 ☔ View full report in Codecov by Sentry. |
| end | ||
|
|
||
| function hash_integer(a::ZZRingElem, h::UInt) | ||
| return _hash_integer(a.d, h) | ||
| return GC.@preserve a _hash_integer(data(a), h) |
There was a problem hiding this comment.
What do you need the preserve here for? a is not allowed to be a ptr by signature
There was a problem hiding this comment.
The return value of data(a) is only useful if a is preserved. (Unless _fmpz_is_small(a) is `true, then it it does not need to be preserved.)
There was a problem hiding this comment.
Indeed: data(a) returns an Int which however is a pointer in disguise. Julia could therefore decide to garbage collect a right after data(a) returned, but before _hash_integer starts. But then we may get garbage or a crash. Of course that only matters if the code calling hash_integer resp. hash does not access the value a anymore afterwards, which seems unlikely -- but not impossible.
| p = unsafe_load(d) | ||
| __fmpz_is_small(a) && return Base.hash_integer(a, h) | ||
| # view it as a BigInt | ||
| z = unsafe_load(Ptr{BigInt}(unsigned(a) << 2)) |
There was a problem hiding this comment.
Doesn't this line need a GC.preserve for a?
There was a problem hiding this comment.
And why not use the new _as_bigint function here?
There was a problem hiding this comment.
Probably all correct, but I don't like the convention that size(Int) = 1, but don't have string feelings about it. For fmpz's this is different and more nuanced due the possibility of beeing immediate....
There was a problem hiding this comment.
This can't use _as_bigint (which BTW is not new, just moved around) because at this point we only have a which is a "pointer-disguised-as-Int.
Note that this code is essentially the same as it was before, I just reorganized it a bit for clarity.
|
Thanks for the explanations. No further comments or objections from my side |
... and their code more readable (at least to me).
Also implement
rem(::ZZRingElem, ::Type{UInt})and use that tospeed up
iseven,isodd,sign,signbitandis_squarefreefor
ZZRingElem.When tested in isolation using BenchmarkTools, the new methods
are usually a little bit faster, and never slower,
than the old ones. The main gain however is in code using them:
unlike opaque
ccalls the compiler can inline these helper andpossibly perform further optimizations.