merge 2 versions (for GMP 5 & 6) of rand(::UnitRange{BigInt}) #13815

rfourquet · 2015-10-29T06:11:19Z

I saw the error caused by incompatible GMP versions (run time vs compile time) reported few times on the lists, but technically the versions need to agree only for rand(::UnitRange{BigInt}), so this PR proposes to remove this constraint.
That said I'm not sure if different versions of GMP could reveal other installation problems, so I kept a warning, please comment on this point.
The second commit optimizes slightly the pre-allocation of the random BigInt in some cases (the speed gains are very modest). This is not related to GMP versions, and I could as well make another PR for that.

vtjnash · 2015-10-29T16:59:51Z

base/gmp.jl

-        ccall((:__gmpz_init,:libgmp), Void, (Ptr{BigInt},), &b)
-        finalizer(b, _gmp_clear_func)
-        return b
+    function BigInt(::Val{:allocbits}, nbits)


allocbits should be a keyword argument

I feared that a keyword would slow things down... the only point of this feature is to improve efficiency by avoiding allocating unnecessarily one Limb (the default of mpz_init) and then re-allocating what is needed from within mpz_* functions. For the same reason, I think sizehint! would not work. Also this is really not meant to be a user API (where a BigInt is immutable), meant only for the wrapping layer of mpz, so the ugliness was kind of "on purpose". Would you accept a solution like BigInt(::Type{Bool}, nbits), or maybe using an ad-hoc type tag BigInt(::Type{SizeHint}, nbits) ? Otherwise, I will update to use a keyword argument.

If it's just going to be internal, we can use a name like BigInt_withsize and switch to a keyword arg later when they are fast.

Ok i'll update using BigInt_withsize. Other methods in gmp.jl may benefit from this constructor.

JeffBezanson · 2015-10-29T17:04:00Z

Instead of allocbits, could this be a method of sizehint!? Val is really ugly.

rfourquet · 2015-10-31T15:17:40Z

So I finally changed Val{:allocbits} to _withsize (which I could change to BigInt_withsize...) but I'm not sure: besides the name, was that the suggested implementation?

rfourquet · 2016-05-22T15:20:59Z

Is _withsize an OK name? Once the name is decided, I can squash the last commit.

tkelman · 2016-05-22T20:16:47Z

base/gmp.jl

+    function BigInt(::_withsize, nbits)
+        z = new(Cint(0), Cint(0), C_NULL)
+        # WARNING: nbits == 0 is unsafe, some mpz_* functions expect an abs size >= 1
+        nbits > 0 && ccall((:__gmpz_init2,:libgmp), Void, (Ptr{BigInt}, Culong), &z, nbits)


should there be an error when nbits <= 0 instead of just doing nothing?

I left this as an option for the programmer to manually initialize the BigIng object at a later stage. Note that as long as the object (equal to big(0)) is not used in a mutative GMP function, this should not be unsafe.

tkelman · 2016-05-22T20:20:04Z

Isn't _withsize just a gmp-specific name for Val? I think Jeff was suggesting using sizehint! for this.

edit: whoops, hadn't read the collapsed conversation

edit2: So rereading it, I think Jeff's suggestion is actually to use a different function name for the _withsize signature.

rfourquet · 2016-05-27T14:19:06Z

Whoops, sorry @tkelman I have missed your last comment. I don't understand how to use a different function name, as there has to be an inner constructor which allows to tune the allocation; otherwise, sizehint! will be fine (1 allocation would be wasted). I agree that _withsize is no more than a gmp-specific name for Val. I could also change the signature to BigInt(_withsize(n)).

tkelman · 2016-05-27T17:24:32Z

Is there currently a meaning to calling BigInt with 3 arguments? Could use the default inner constructor as an unchecked shortcut, make the existing 0 arg method an outer constructor, then use different names for preallocated versions?

rfourquet · 2016-05-29T06:52:59Z

Ah I get it now, thanks. It's not my preference to introduce a new name for what is actually a constructor but I updated accordingly. The 3-argument constructor is left unchecked (in particular with no finalizer attached). Another option: using a dummy Bool value in a 2-argument inner constructor, eg BigInt(withsize::Bool, size::Int, ) where withsize is not used.

rfourquet · 2016-06-09T11:57:53Z

Would it help pushing this forward if I entirely remove the BigInt_withsize business? I somehow need to control the allocated size, but I guess this could be accomplished by something like z = big(2)^bits_to_allocate.

rfourquet · 2016-06-15T14:09:22Z

I rebased because of merge conflicts. I decided to remove the BigInt_withsize constructor, it is not essential here, and I instead ccall'ed to libgmp to allocate when needed. If I can defend the introduction of a BigInt constructor which controls allocation with performance numbers, it will be in another PR.
There was an apparently not related error on travis with the previous version, which I don't understand (test blas, "AssertionError: frame.inferred"), here is the link.

rfourquet · 2017-05-20T14:59:57Z

Rebased. I would like to merge soon, if no-one objects...

rfourquet · 2017-05-23T08:38:40Z

Good to go?

tkelman · 2017-05-26T14:23:40Z

you've tested this against older libgmp?

tkelman · 2017-05-27T07:08:24Z

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

rfourquet · 2017-05-27T10:48:00Z

I didn't think to test against older gmp versions, good idea, I'm running the whole tests locally now with gmp-5.1.3 (you must have seen I had rebased, I was about to merge!)

tkelman · 2017-05-27T10:52:09Z

cc @nalimilan as well who I believe maintains some configurations of his copr against older libgmp, main user of the older branch code

rfourquet · 2017-05-27T11:29:01Z

There is only one failure with gmp-5.1.3, on numbers, which seems unrelated: powermod(1, -1, big(1)) == 0 throws an integer division error (in this branch as well as on master).

nanosoldier · 2017-05-27T12:54:14Z

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

ViralBShah · 2017-05-28T20:49:26Z

I think we can drop gmp-5 support, if it simplifies things.

rfourquet · 2017-05-29T13:45:46Z

This PR doesn't change the compatibility with gmp-5, it bypasses the different APIs by working directly with the low-level representation. On the other hand, the error with powermod I mentioned above seems to imply that gmp-5 is not 100% supported.
So I will finally merge tomorrow if no objection.

As a consequence, using dynamically a version of GMP which does not match the compile time version is no longer an error (this patch makes this a warming). The code is shorter, but we break the MPZ API boundary; this also allows to put allocating pointer_to_array out of the loop, and get slightly better performances.

vtjnash reviewed Oct 29, 2015
View reviewed changes

rfourquet force-pushed the rand-bigrange-mergeversions branch 2 times, most recently from 52e3144 to 04d780f Compare May 22, 2016 15:13

tkelman reviewed May 22, 2016
View reviewed changes

rfourquet force-pushed the rand-bigrange-mergeversions branch from 04d780f to 28286fb Compare May 29, 2016 06:52

rfourquet force-pushed the rand-bigrange-mergeversions branch from 28286fb to 47aee88 Compare June 15, 2016 14:00

rfourquet added the randomness Random number generation and the Random stdlib label Jun 15, 2016

rfourquet force-pushed the rand-bigrange-mergeversions branch from 47aee88 to f8223cd Compare June 19, 2016 07:29

rfourquet mentioned this pull request Jun 19, 2016

[WIP] implement BigInt (and BigFloat?) with native Array #17015

Closed

rfourquet force-pushed the rand-bigrange-mergeversions branch from f8223cd to 765febf Compare May 20, 2017 14:58

rfourquet mentioned this pull request May 20, 2017

support pre-allocation in BigInt ctor #21983

Merged

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

StefanKarpinski approved these changes May 23, 2017

View reviewed changes

rfourquet force-pushed the rand-bigrange-mergeversions branch from 765febf to 2ca7166 Compare May 26, 2017 14:00

rfourquet added 2 commits June 3, 2017 12:25

avoid re-allocations in rand(UnitRange(::BigInt)) for some cases

141a8f4

rfourquet force-pushed the rand-bigrange-mergeversions branch from 2ca7166 to 141a8f4 Compare June 3, 2017 10:25

rfourquet merged commit 063e8f1 into JuliaLang:master Jun 3, 2017

rfourquet deleted the rand-bigrange-mergeversions branch June 3, 2017 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

merge 2 versions (for GMP 5 & 6) of rand(::UnitRange{BigInt}) #13815

merge 2 versions (for GMP 5 & 6) of rand(::UnitRange{BigInt}) #13815

rfourquet commented Oct 29, 2015

vtjnash Oct 29, 2015

rfourquet Oct 30, 2015

JeffBezanson Oct 30, 2015

rfourquet Oct 30, 2015

JeffBezanson commented Oct 29, 2015

rfourquet commented Oct 31, 2015

rfourquet commented May 22, 2016

tkelman May 22, 2016

rfourquet May 23, 2016 •

edited

Loading

tkelman commented May 22, 2016 •

edited

Loading

rfourquet commented May 27, 2016

tkelman commented May 27, 2016

rfourquet commented May 29, 2016

rfourquet commented Jun 9, 2016

rfourquet commented Jun 15, 2016

rfourquet commented May 20, 2017

rfourquet commented May 23, 2017

tkelman commented May 26, 2017

tkelman commented May 27, 2017

rfourquet commented May 27, 2017

tkelman commented May 27, 2017

rfourquet commented May 27, 2017

nanosoldier commented May 27, 2017

ViralBShah commented May 28, 2017

rfourquet commented May 29, 2017

merge 2 versions (for GMP 5 & 6) of rand(::UnitRange{BigInt}) #13815

merge 2 versions (for GMP 5 & 6) of rand(::UnitRange{BigInt}) #13815

Conversation

rfourquet commented Oct 29, 2015

vtjnash Oct 29, 2015

Choose a reason for hiding this comment

rfourquet Oct 30, 2015

Choose a reason for hiding this comment

JeffBezanson Oct 30, 2015

Choose a reason for hiding this comment

rfourquet Oct 30, 2015

Choose a reason for hiding this comment

JeffBezanson commented Oct 29, 2015

rfourquet commented Oct 31, 2015

rfourquet commented May 22, 2016

tkelman May 22, 2016

Choose a reason for hiding this comment

rfourquet May 23, 2016 • edited Loading

Choose a reason for hiding this comment

tkelman commented May 22, 2016 • edited Loading

rfourquet commented May 27, 2016

tkelman commented May 27, 2016

rfourquet commented May 29, 2016

rfourquet commented Jun 9, 2016

rfourquet commented Jun 15, 2016

rfourquet commented May 20, 2017

rfourquet commented May 23, 2017

tkelman commented May 26, 2017

tkelman commented May 27, 2017

rfourquet commented May 27, 2017

tkelman commented May 27, 2017

rfourquet commented May 27, 2017

nanosoldier commented May 27, 2017

ViralBShah commented May 28, 2017

rfourquet commented May 29, 2017

rfourquet May 23, 2016 •

edited

Loading

tkelman commented May 22, 2016 •

edited

Loading