speed-up prevpow2/nextpow2 #21231

rfourquet · 2017-03-30T09:24:59Z

The speed-up comes mainly from avoiding creating unnecessarily BigInt temporaries.
The last commit introduces ONE and ZERO which are BigInt constants with obvious values. The WIP here is because I don't know of a good way to initialize them: I tried to ccall the mpz_set_si function from within the __init__ function, but then get a segmentation fault when launching julia (my current workaround is to use an array, or could use a malloc). Suggestions welcome.

tkelman · 2017-03-30T09:34:04Z

Do you know if this would be covered by any performance tests in BaseBenchmarks?

thofma · 2017-03-30T14:47:29Z

base/gmp.jl

@@ -458,7 +475,7 @@ powermod(x::Integer, p::Integer, m::BigInt) = powermod(big(x), big(p), m)

 function gcdx(a::BigInt, b::BigInt)
    if b == 0 # shortcut this to ensure consistent results with gcdx(a,b)
-        return a < 0 ? (-a,-one(BigInt),zero(BigInt)) : (a,one(BigInt),zero(BigInt))
+        return a < 0 ? (-a,-ONE,ZERO) : (a,ONE,ZERO)


I don't think that is a good idea. Mutating the return values of gcdx(BigInt(1), BigInt(0)) should not change the behavior of gcdx or any of the other functions that use ONE or ZERO.

I'm not sure what to think about this, as mutation of BigInt is not covered by the public API. For reference, there was a discussion about "alias-stability" in another issue.

It has nothing to do with aliasing.

If I do a,b,c = gcdx(BigInt(1), BigInt(0)) and I do f(b) which mutates b (which is not forbidden, since it is mutable), then the global variable ONE will be changed.

Again, this has nothing to do with aliasing.

The topic in the link (aliasing between arguments and return values of functions) is slightly different from the problem here (aliasing between arguments and global objects), but still very connected, as it refers to the safety of mutating the return value of a function (and whether it is always safe (cf. e.g. "computing something always involves returning a new object" in that thread) or depends on the runtime value of the argument).
Again as mutating BigInts is not part of the public API (even if of course some code will mutate them for efficiency, as the language can not yet enforce their technical immutability), I think my aliasing code is OK, but I agree this is debatable (in particular as the expected speed-up here is not tremendous and concerns only the case of b==0).

I don't think it's OK. I don't know why you think #15546 is supporting this change here. In #15546 no decision was made about the whole aliasing thing, but people were complaining about undefined aliasing behavior, that is, runtime dependent aliasing behavior. Here you introduce yet another unnecessary suprising aliasing behavior, which will yield to very subtle bugs.

If julia wraps GMP's mpz as BigInt, then it should be possible to call any function from GMP with BigInts. With this change this is not safe anymore. You would always have to look up, if the wrap julia function is returning the newly introduced global variables. And I am not talking about public API, but also about future modifications of the GMP module.

a,b,c = gcd(BigInt(1), BigInt(0)) set_zero!(b)

This code snippet should never change the behavior of julia, but with the proposed change it will. It leads to runtime changes in all the functions calling ONE and ZERO.

I didn't say the issue I linked to was supporting this change, I was merely referencing it as a relevant discussion for the problem you pointed at. BigInt is considered to be conceptually (if not technically) an immutable type, and it's possible that a future version of Julia gets BigInt implemented as immutable/struct (cf. e.g. https://github.com/JuliaLang/julia/pull/17015/files#diff-c39f3bbf0cb1c2381243b42647d9ff1dR43), which would break the mutating code you posted above. I don't have a strong feeling about what should be the resolution here, and will gladly modify the code according to the consensus if any is reached.

I would love to see something along the lines of #17015 being merged. But in this case, GMP.jl would have to be rewritten anyways (and this change here would be unnecessary).

Since such a change did not take place, the only way to write fast code for BigInts is to directly call the corresponding gmp functions. And this works pretty well at the moment. But with the changes here, this is more error prone.

While the speedup is negligible, with this change mutating a mutable type can change global variables and consequently the behavior of various functions.

Would you be fine with return a < 0 ? (-a,-ONE,b) : (a,one(BigInt),b), where b == 0, such that the return value would alias an argument?

Sounds good to me.

rfourquet · 2017-03-31T08:38:00Z

@tkelman by grepping through BaseBenchmarks, I don't see those functions covered. I could issue a PR to include them. Very roughly, the speed-up here is about 100% or 150%.

rfourquet · 2017-04-09T04:21:13Z

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

nanosoldier · 2017-04-09T05:11:04Z

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

rfourquet · 2017-04-22T11:31:00Z

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

nanosoldier · 2017-04-22T12:20:27Z

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

rfourquet · 2017-04-25T15:24:25Z

Nanosoldier confirmed speed improvements. I rebased with minor updates, this would be ok to go on my side. Must this wait till after the next release?

musm · 2017-04-25T15:27:36Z

base/gmp.jl

@@ -518,6 +534,12 @@ iszero(x::BigInt) = x == Clong(0)
 <(i::Integer, x::BigInt) = cmp(x,i) > 0
 <(x::BigInt, f::CdoubleMax) = isnan(f) ? false : cmp(x,f) < 0
 <(f::CdoubleMax, x::BigInt) = isnan(f) ? false : cmp(x,f) > 0
+isneg(x::BigInt) = x.size < 0
+ispos(x::BigInt) = x.size > 0


Is it necessary to introduce these two function, looks like ispos is only used in one location and isneg in two locations?

No not necessary at all, only I don't like this kind of lowlevel considerations when writing an algorithm; also I thought it could be used elsewhere, e.g. in "base/printf.jl".

what about ispositive and isnegative (ref #20484 (comment))
might be better more explicitly named?

Why not, but as isposdef is already a function name, ispos could be good enough. If your suggestion receives support, I will update.

If we're adding these functions, should we also support them for Integer, or might that come in the future if needed?

👍 for writing positive and negative out in full.

I think it would be useful indeed to define these functions more generally for Integer, in the same way that iszero is defined (it "may" increase performances...), but I prefer not in this PR.
I'm still a bit reluctant with writing ispositive and isnegative out in full: x > 0 is so much easier to write! also, what should we call x >= 0 ? ispositiveoreq(x) (or isposeq(x)) ? I'm wondering now if it wouldn't be simpler to check for the value 0 in the implementation of <(x::BigInt, y::Int) etc.

rfourquet · 2017-04-30T10:02:53Z

Bump, this shouldn't take too long to review!

Sacha0 · 2017-04-30T16:10:53Z

base/gmp.jl

@@ -104,7 +111,7 @@ function tryparse_internal(::Type{BigInt}, s::AbstractString, startpos::Int, end
        raise && throw(ArgumentError("invalid BigInt: $(repr(bstr))"))
        return _n
    end
-    Nullable(sgn < 0 ? -z : z)
+    Nullable(flipsign!(z, sgn))


Should the ! be there? The existing statement does not seem to modify z?

As z was created in the function, it "owns" this object and hence is free to mutate it for optimization, which is the main purpose of this PR: mutating BigInt objects in place when possible to avoid expensive copies.

Sacha0 · 2017-04-30T16:17:35Z

base/gmp.jl

-        return a < 0 ? (-a,-one(BigInt),zero(BigInt)) : (a,one(BigInt),zero(BigInt))
+    if iszero(b) # shortcut this to ensure consistent results with gcdx(a,b)
+        return a < 0 ? (-a,-ONE,b) : (a,one(BigInt),b)
+        # we don't return the globals ONE and ZERO in case the user wants to


This comment is somewhat confusing: The comment states that you do not return ONE. Yet one of the branches in the line immediately preceding the comment returns ONE?

In the preceding line, the return values are either one(BigInt) or -ONE, both of these expressions create a freshly allocated BigInt (as requested by @thofma).

Ah, the -! Perfect. Thanks for the explanation!

ViralBShah · 2017-05-03T11:45:16Z

Is this good to merge now? Perhaps @JeffBezanson or @simonbyrne.

rfourquet · 2017-05-03T11:53:57Z

This is good to merge on my side, modulo a decision regarding ispos vs ispositive (as it is not exported, it may not be a problem to report this decision until those functions become defined for Integer and documented).

Avoid a library call into GMP.

rfourquet · 2017-05-10T10:49:58Z

Bump

StefanKarpinski · 2017-05-11T12:24:17Z

This seems applicable to 0.6 since it doesn't change any visible behavior as far as I can tell?

tkelman · 2017-05-11T12:28:05Z

it's implementing several new methods on BigInt which weren't present in alpha, beta, or rc1

rfourquet · 2017-05-11T14:23:43Z

Besides those few (unexported) new methods on BigInt, I think indeed that it doesn't change any visible behavior.

StefanKarpinski · 2017-05-12T04:22:34Z

If the new methods implement the same behavior as before, it's not actually breaking.

tkelman · 2017-05-12T07:38:09Z

it's the imported methods that are now globally visible that weren't previously implemented at all - not breaking unless someone was relying on them not working, but a new feature

rfourquet · 2017-05-12T08:39:46Z

If I'm not mistaken, only flipsign and signbit are imported now, but only to implement a more efficient version, as the functionality was already implemented generically (and available for BigInt).
If this is to be backported, can I do something to help?

Avoid a library call into GMP. (cherry picked from commit 05708f8) ref #21231 speed-up prevpow2 & nextpow2 for BigInt (cherry picked from commit 42fd2a7) GMP: add flipsign[!] functions and use more iszero/isneg/ispos (cherry picked from commit f63bdfd) add ZERO & ONE global BigInt constants for optimization (cherry picked from commit b521acd)

thofma reviewed Mar 30, 2017

View reviewed changes

ararslan added domain:maths Mathematical functions performance Must go faster labels Mar 30, 2017

rfourquet force-pushed the rf/bigint-prevpow2 branch from 3848185 to 25aa067 Compare March 31, 2017 06:17

rfourquet mentioned this pull request Apr 1, 2017

add prevpow2 and nextpow2 benchmarks JuliaCI/BaseBenchmarks.jl#70

Merged

rfourquet force-pushed the rf/bigint-prevpow2 branch from 25aa067 to 3f0a8c8 Compare April 9, 2017 05:57

jrevels mentioned this pull request Apr 10, 2017

retune benchmarks (includes #70) JuliaCI/BaseBenchmarks.jl#72

Merged

rfourquet force-pushed the rf/bigint-prevpow2 branch from 3f0a8c8 to 2dc3a9f Compare April 25, 2017 15:22

rfourquet changed the title ~~WIP: speed-up prevpow2/nextpow2~~ speed-up prevpow2/nextpow2 Apr 25, 2017

musm reviewed Apr 25, 2017

View reviewed changes

rfourquet force-pushed the rf/bigint-prevpow2 branch from 2dc3a9f to c6b7c86 Compare April 26, 2017 13:59

Sacha0 reviewed Apr 30, 2017

View reviewed changes

rfourquet added 4 commits May 10, 2017 12:39

make iszero(::BigInt) faster

05708f8

Avoid a library call into GMP.

speed-up prevpow2 & nextpow2 for BigInt

42fd2a7

GMP: add flipsign[!] functions and use more iszero/isneg/ispos

f63bdfd

add ZERO & ONE global BigInt constants for optimization

b521acd

rfourquet force-pushed the rf/bigint-prevpow2 branch 2 times, most recently from d241467 to b521acd Compare May 10, 2017 10:49

StefanKarpinski merged commit 7ea1620 into master May 11, 2017

StefanKarpinski deleted the rf/bigint-prevpow2 branch May 11, 2017 12:23

StefanKarpinski added the backport pending 0.6 label May 11, 2017

tkelman removed the backport pending 0.6 label May 18, 2017

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

speed-up prevpow2/nextpow2 #21231

speed-up prevpow2/nextpow2 #21231

Conversation

rfourquet commented Mar 30, 2017

tkelman commented Mar 30, 2017

thofma Mar 30, 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

thofma Apr 1, 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

rfourquet commented Mar 31, 2017

rfourquet commented Apr 9, 2017

nanosoldier commented Apr 9, 2017

rfourquet commented Apr 22, 2017

nanosoldier commented Apr 22, 2017

rfourquet commented Apr 25, 2017

musm Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm Apr 26, 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

rfourquet commented Apr 30, 2017

Choose a reason for hiding this comment

rfourquet Apr 30, 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

ViralBShah commented May 3, 2017

rfourquet commented May 3, 2017

rfourquet commented May 10, 2017

StefanKarpinski commented May 11, 2017

tkelman commented May 11, 2017

rfourquet commented May 11, 2017

StefanKarpinski commented May 12, 2017

tkelman commented May 12, 2017

rfourquet commented May 12, 2017

thofma Mar 30, 2017 •

edited

Loading

thofma Apr 1, 2017 •

edited

Loading

musm Apr 25, 2017 •

edited

Loading

musm Apr 26, 2017 •

edited

Loading

rfourquet Apr 30, 2017 •

edited

Loading