group ccall'ed MPZ library functions in a module #21654

rfourquet · 2017-04-30T17:14:28Z

The goals where 1) making the code cleaner in the implementation of julia function (so that it's not full of difficult-to-read ccalls), and 2) avoid duplication for for some of the ccall, within base itself, but also for users/packages.

With the help of some functions which create automatically new BigInt objects for output arguments, the simplification in 1) has gone beyond my expectation :) The same could probably be done in mpfr.jl.

ararslan · 2017-04-30T18:55:27Z

base/gmp.jl

@@ -69,6 +69,93 @@ function __init__()
    end
 end

+
+module mpz


Modules names should be in all caps (if an acronym) or upper camel case

Yes I'm sensible to that, but I made this choice to be as close as possible to the original function names, of the form "mpz_add", which translates here into "mpz.add". An alternative could be to not have any submodule and to name the julia functions as in GMP, with the "mpz_" prefix. I prefer the module with lower case, and I thought that an exception to the style can be made as it's an internal sub-sub-module of Base. But will change if you or someone insists.

would rather stick with the established naming conventions. I don't think it's especially necessary to have julia wrappers for C functions use exactly the same names as in the C library, when the conventions or meanings for names differ in idiomatic Julia

Agree, most importantly because the convention can easily lead one to believe when perusing the code that mpz is an object rather than a module, which is quite confusing. So I think that writing MPZ.add would be less likely to be confusing.

Ok I got convinced :) thanks for your input.

ararslan · 2017-04-30T19:00:45Z

This is cool!

@nanosoldier runbenchmarks(ALL, vs=":master")

musm · 2017-04-30T19:25:50Z

base/gmp.jl

+end
+
+invert(x::BigInt, a::BigInt, b::BigInt) =
+    ccall((:__gmpz_invert, :libgmp), Cint, (Ptr{BigInt}, Ptr{BigInt}, Ptr{BigInt}), &x, &a, &b)


If this is getting updated might as well update the & syntax , since it is deprecated and should be using the Ref{T} argument type instead.

it still has a performance cost though

Will as well then wait till the performances get on par...

rfourquet · 2017-05-01T08:49:53Z

@nanosoldier runbenchmarks(("BigInt"||"Complex{BigInt}"), vs=":master")

rfourquet · 2017-05-01T08:52:41Z

@ararslan thanks, your nanosoldier run didn't work probably because I had a forgotten import in random.jl. Running again with filters now, hope this covers all what is necessary.

jrevels · 2017-05-01T14:45:25Z

@rfourquet You need commit access to trigger nanosoldier (maybe you already have access, but that's my guess as to why nothing happened here, since everything looks fine server side).

@nanosoldier runbenchmarks("BigInt" || "Complex", vs=":master")

jrevels · 2017-05-01T14:48:37Z

Weird, nothing's happening here even though the logs server-side seem fine. I kicked Nanosoldier, let's try again:

@nanosoldier runbenchmarks("BigInt" || "Complex", vs=":master")

nanosoldier · 2017-05-01T15:38:04Z

Your benchmark job has completed, but no benchmarks were actually executed. Perhaps your tag predicate contains mispelled tags? cc @jrevels

jrevels · 2017-05-01T17:08:13Z

Benchmarks do exist with those keys, but keys weren't counted as tags in BenchmarkTools until v0.0.8, and Nanosoldier only had v0.0.7. Just did a Pkg.update, let's see how it goes:

@nanosoldier runbenchmarks("BigInt" || "Complex", vs=":master")

nanosoldier · 2017-05-01T18:03:56Z

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels

rfourquet · 2017-05-02T08:52:31Z

Thanks @jrevels ! Rerunning with "Complex{BigInt}", as it seems "Complex" is not enough to handle it.

rfourquet · 2017-05-02T08:53:18Z

@nanosoldier runbenchmarks("Complex{BigInt}", vs=":master")

nanosoldier · 2017-05-02T09:47:45Z

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels

thofma · 2017-05-02T19:58:16Z

base/gmp.jl

+
+for op in (:add, :sub, :mul, :fdiv_q, :tdiv_q, :fdiv_r, :tdiv_r, :gcd, :lcm, :and, :ior, :xor)
+    @eval begin
+        $op(x::BigInt, a::BigInt, b::BigInt) = (ccall($(gmpz(op)), Void, (Ptr{BigInt}, Ptr{BigInt}, Ptr{BigInt}), &x, &a, &b); x)


Should get a ! since it is mutating the input.

Thanks for your review. I hesitated about that, but prefered to follow as closely as possible the original names (from GMP). Am fine to change though; according to their previous comments on the case of the module name, I guess @tkelman and @StefanKarpinski would agree with you?

There is also the "problem" that since we are extending Base.gcd, there will also be a method gcd(::BigInt, ::BigInt, ::BigInt) exposed to the user, which does funny things from a user perspective. In particular it is breaking the following behaviour:

julia> gcd(BigInt(3), BigInt(2), BigInt(4)) 1

With the proposed change the function mutates the first argument and returns 2.

~~I don't think this is what we want and gcd is not the only function with this problem.~~

Edit: Sorry, my mistake. This does not break anything.

Yes you are right, though it is mitigated by the fact that Base.gcd is not touched here, we introduce only a hard-to-find Base.GMP.MPZ.gcd.

Sorry, you are right. Overlooked the MPZ module.

@StefanKarpinski and @tkelman, could you comment on this? I kept the names as in libgmp without any ! even for mutating functions (and for few functions I added a variant with a ! when updating the input, like add!(x, y) = add(x, x, y), equivalent to a x += y), which seems more predictable for me, but @thofma suggests to add ! whenever an argument is modified.

I think Julia wrapper functions should try to aim for following idiomatic Julia naming/argument conventions whenever possible, rather than matching C API naming exactly (if you want to be calling the C API directly, you can always use ccall yourself). We have a bit of an unfortunate mix of these styles at the moment, I have a few of the library binding files on my personal to-do list to clean up in this way and be more consistent about. Disparate C libraries aren't going to have any common conventions, but would be nice if our bindings attempted to.

Thank you again for your extended answer, I will update accordingly.

thofma · 2017-05-02T19:59:05Z

base/gmp.jl

+
+for op in (:add_ui, :sub_ui, :mul_ui, :mul_2exp, :fdiv_q_2exp, :pow_ui, :bin_ui)
+    @eval begin
+        $op(x::BigInt, a::BigInt, b) = (ccall($(gmpz(op)), Void, (Ptr{BigInt}, Ptr{BigInt}, Culong), &x, &a, b); x)


thofma · 2017-05-02T19:59:26Z

base/gmp.jl

+    end
+end
+
+ui_sub(x::BigInt, a, b::BigInt) = (ccall((:__gmpz_ui_sub, :libgmp), Void, (Ptr{BigInt}, Culong, Ptr{BigInt}), &x, a, &b); x)


thofma · 2017-05-02T19:59:47Z

base/gmp.jl

+
+for op in (:neg, :com, :sqrt, :set)
+    @eval begin
+        $op(x::BigInt, a::BigInt) = (ccall($(gmpz(op)), Void, (Ptr{BigInt}, Ptr{BigInt}), &x, &a); x)


thofma · 2017-05-02T19:59:59Z

base/gmp.jl

+
+popcount(a::BigInt) = Int(ccall((:__gmpz_popcount, :libgmp), Culong, (Ptr{BigInt},), &a))
+
+function tdiv_qr(x::BigInt, y::BigInt, a::BigInt, b::BigInt)


thofma · 2017-05-02T20:01:13Z

There are more functions missing a !.

rfourquet · 2017-05-12T10:13:03Z

I updated by renaming functions with a ! when mutating as requested. I'm not sure for the function MPZ.get_str which writes to a passed char* without modifying the pointer itself. Still, as a String can be passed in instead (via a unsafe_convert) and be modified by this function, I added also a !.

tkelman · 2017-05-13T23:22:16Z

This looks good to me (I made one trivial edit to a comment) - these look like they would be handy for writing in-place BigInt code. Let's run @nanosoldier runbenchmarks(ALL, vs=":master") again, why not

nanosoldier · 2017-05-14T02:18:39Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

rfourquet · 2017-05-14T10:23:59Z

Thanks. Indeed working on in-place BigInt code in Primes.jl pulled me to make this PR.
The regressions reported by nanosoldier seem unrelated to this patch.
I forgot to discuss: we use (only) one GMP function prefixed with "mpn_" instead of "mpz_" (i.e. "mpn_popcount"), so I put it in this MPZ module for simplicity, but it could be cleaner to create a separate MPN module. It may depend on whether or not this module has vocation to become exhaustive (implementing ccall's which are not used by base itself, to serve package authors). What do you think?

tkelman · 2017-05-14T11:13:33Z

What's the difference between mpz and mpn in libgmp? Hard to keep track of their naming convention if you don't use the low-level library calls that often. There's no problem with packages writing extended library bindings to libraries that are for now shipped by default with Julia (see for example BandedMatrices.jl which has some lapack bindings that aren't all that useful within base), but the eventual plan is probably to separate out the library-specific bindings from the bare-minimum BigInt type definition, and make the former optional or swappable (if we ever care more about MSVC we may want to try out MPIR instead of libgmp for example).

rfourquet · 2017-05-14T11:44:16Z

The MPN functions are lower level than MPZ: MPZ tries to provide a convenient application-level user interface, while MPN provides low-level routines taking raw arrays as input instead of mpz_t objects (i.e. BigInt), which can be used by library implementors to roll-up their own bigint objects. For example, I started some time ago to experiment with a different layout for BigInt, which would require using only MPN functions, but migrating away from MPZ to MPN-only is already a big work.
According to your answer, it seems fine for now to keep this mpn_popcount function in the MPZ namespace for simplicity.

rfourquet · 2017-05-15T17:18:17Z

I added a forgotten binding used in the BigInt constructor. If this receives no more comments, I think this should be safe to merge within few days.

Jutho · 2017-10-13T10:44:20Z

Thanks for this nice PR. I use the inplace version of BigInt (i.e. the new MPZ module) in https://github.com/Jutho/WignerSymbols.jl . How difficult would it be to backport this to Julia v0.6, by which I not mean officially, but just within my package.

Would it be sufficient to copy all the code between module MPZ ... end into my package, sandwiched between a version check? Is that allowed (in terms of license restrictions)?

rfourquet · 2017-10-14T17:44:28Z

Thanks! I had not realized this change was not in 0.6. It should be pretty trivial to backport to 0.6, indeed by copying all the code in the MPZ module (maybe changing a few things, like const mpz_t = Ref{BigInt} back to const mpz_t = Ptr{BigInt}). Let me know if you have any trouble. Licence-wise, I have unfortunately no idea, but if "MIT-expat" (your package) is compatible with Julia's "MIT licence" it should be fine?

Jutho · 2017-10-14T18:33:41Z

Thanks for your help. Everything seems to work indeed by just copying the MPZ module, even without any change. I would indeed think that licenses are compatible, so I'll take my chance and register the package including a copy of your MPZ module, if that's fine by you. When Julia 0.7 or 1.0 is released, I'll probably drop 0.6 support.

rfourquet force-pushed the rf/big-ccall branch from cb7cb53 to fd6ea75 Compare April 30, 2017 18:40

ararslan reviewed Apr 30, 2017

View reviewed changes

ararslan requested a review from simonbyrne April 30, 2017 18:58

ararslan added the maths Mathematical functions label Apr 30, 2017

musm reviewed Apr 30, 2017

View reviewed changes

rfourquet force-pushed the rf/big-ccall branch from fd6ea75 to 3146bdd Compare May 1, 2017 07:31

rfourquet force-pushed the rf/big-ccall branch from 78999e3 to adfd813 Compare May 2, 2017 13:28

thofma reviewed May 2, 2017

View reviewed changes

rfourquet force-pushed the rf/big-ccall branch 5 times, most recently from 44c6719 to 662439a Compare May 12, 2017 10:12

rfourquet force-pushed the rf/big-ccall branch from 662439a to c9823e4 Compare May 12, 2017 12:45

rfourquet force-pushed the rf/big-ccall branch from 80f72fb to 601b87e Compare May 15, 2017 17:15

StefanKarpinski approved these changes May 16, 2017

View reviewed changes

group ccall'ed MPZ library functions in a module

9fefc95

rfourquet force-pushed the rf/big-ccall branch from b6fce75 to 9fefc95 Compare May 19, 2017 09:34

rfourquet merged commit 9a477f2 into master May 19, 2017

rfourquet deleted the rf/big-ccall branch May 19, 2017 11:00

rfourquet added the bignums BigInt and BigFloat label May 23, 2017

rfourquet mentioned this pull request May 28, 2017

printf.jl: add missing import of GMP #22104

Merged

rfourquet mentioned this pull request Sep 27, 2019

rename perm -> Perm Nemocas/AbstractAlgebra.jl#433

Merged


		popcount(a::BigInt) = Int(ccall((:__gmpz_popcount, :libgmp), Culong, (Ptr{BigInt},), &a))

		function tdiv_qr(x::BigInt, y::BigInt, a::BigInt, b::BigInt)

group ccall'ed MPZ library functions in a module #21654

group ccall'ed MPZ library functions in a module #21654

Conversation

rfourquet commented Apr 30, 2017

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

Choose a reason for hiding this comment

ararslan commented Apr 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfourquet commented May 1, 2017

rfourquet commented May 1, 2017 • edited Loading

jrevels commented May 1, 2017 • edited Loading

jrevels commented May 1, 2017

nanosoldier commented May 1, 2017

jrevels commented May 1, 2017

nanosoldier commented May 1, 2017

rfourquet commented May 2, 2017

rfourquet commented May 2, 2017

nanosoldier commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thofma May 3, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thofma commented May 2, 2017

rfourquet commented May 12, 2017

tkelman commented May 13, 2017

nanosoldier commented May 14, 2017

rfourquet commented May 14, 2017

tkelman commented May 14, 2017

rfourquet commented May 14, 2017 • edited Loading

rfourquet commented May 15, 2017

Jutho commented Oct 13, 2017

rfourquet commented Oct 14, 2017

Jutho commented Oct 14, 2017

rfourquet commented May 1, 2017 •

edited

Loading

jrevels commented May 1, 2017 •

edited

Loading

thofma May 3, 2017 •

edited

Loading

rfourquet commented May 14, 2017 •

edited

Loading