-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Improve primes
performance.
#11927
Conversation
I didn't intend to replace Also julia> methods(primes)
# 1 method for generic function "primes":
primes(n::Union{AbstractArray{Bool,1},Integer}) at primes.jl:33 And I don't know how to support |
The implementation of |
I tested you version and I see no performance difference between I was reading that the algorithm from https://en.wikipedia.org/wiki/Sieve_of_Atkin#Pseudocode , should perform better that the one in base or yours for that matter. Actually, from the discussion in http://stackoverflow.com/questions/19388106/the-sieve-of-atkin/20572948#20572948 it seems that the following algorithm should be better if you want to use a Sieve of Atkin.
It also seems from the last link that an optimized Sieve of Eratosthenes whit a higher degree of wheel factorization might perform even better. |
Although, the asymptotic behaviour of your algorithm or the one in base should be better as indicated in Atkin's paper (http://www.ams.org/mcom/2004-73-246/S0025-5718-03-01501-1/S0025-5718-03-01501-1.pdf) Here http://stackoverflow.com/questions/19388106/the-sieve-of-atkin/20572948#20572948 it is claimed that this Sieve of Atkin is probably better for very high values of the number of primes. |
I don't know why I'm getting a This CI test (and others) failed:
But this one passed. At first I thought it was because of the use of |
You are testing if |
primes
performance.
primes = Int[] | ||
|
||
elseif limit == 2 | ||
primes = [2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
limit < 0 && error("limit can't be negative (found $(limit))")
limit < 2 && return primes = Int[]
limit == 2 && return primes = [2]
@StefanKarpinski thanks got it! @pabloferz thanks! I've updated the PR. How did you tested?, I could also try to implement the wheel and fragmented versions and also a sieve of Eratosthenes and we could pick from both: I'm not much of a math guy, do you know what does |
@StefanKarpinski how does julia> for n in 1:100
@show n
@show BitVector(n) == falses(n)
println()
end
...
n = 64
BitVector(n) == falses(n) = true
n = 65
BitVector(n) == falses(n) = false
... |
Using the |
Does julia have any "compressed" bit vector format? (Ie. that can handle sparse arrays of bits efficiently?) |
@Ismael-VC I checked again and can confirm the performance improvement. However, you should really improve the I found that the performance hit in the current implementation comes from this lines i, j, k = 4xx+yy, 3xx+yy, 3xx-yy
i <= n && (s[i] $= (i%12==1)|(i%12==5))
j <= n && (s[j] $= (j%12==7))
1 <= k <= n && (s[k] $= (x>y)&(k%12==11)) By changing this to something similar to what you have, let say (k = 4xx+yy) ≤ n && (k % 12 == 1 || k % 12 == 5) && (s[k] = !s[k])
(k -= xx ) ≤ n && (k % 12 == 7 ) && (s[k] = !s[k])
(k -= 2yy); 1 ≤ k ≤ n && x > y && (k % 12 == 11) && (s[k] = !s[k]) you get the same performance as in your |
k = 3x² - y² | ||
if x > y && k ≤ n && k % 12 == 11 | ||
@inbounds s[k] = !s[k] | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the same variable instead of the three i
, j
and k
. Also, you can move the @inbounds
to the beginning of the loop to have
@inbounds for x = 1:r; x² = x*x
# Rest of the code
end
instead putting one every time you set an index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it makes no difference in the expanded expression and I have seen both styles being used, that's why at the start I used:
function foo()
@inbounds begin
# ...
end
end
But something felt wrong about it. I think it could hide bugs.
I dream every night that I have the official™ julian style guide in my hands ...but then I wake up! 😟
@quinnj thanks! I didn't know about that. @pabloferz yes I know, but I wanted to make sure that it was indeed better even if it was for a bit. I wil try to implement the wheel next, but what does
Now that the documentation is being redesigned, is there a way we could produce a low level (just for devs, for both exported and non exported objects) API documentation?, or at least comment with a mnemonic name all the short variables, ie. for x = 1:r; x² = x*x
for y = 1:r; y² = y*y
i = 4x² + y²
if i ≤ n && (i % 12 == 1 || i % 12 == 5)
sieve[i] = !sieve[i]
end
j = 3x² + y²
if j ≤ n && j % 12 == 7
sieve[j] = !sieve[j]
end
k = 3x² - y²
if x > y && k ≤ n && k % 12 == 11
sieve[k] = !sieve[k]
end
end
end Is IMHO the last snippet is better than: for x = 1:r
xx = x*x
for y = 1:r
yy = y*y
i, j, k = 4xx+yy, 3xx+yy, 3xx-yy
i <= n && (s[i] $= (i%12==1)|(i%12==5))
j <= n && (s[j] $= (j%12==7))
1 <= k <= n && (s[k] $= (x>y)&(k%12==11))
end
end What does And certainly better than: (k = 4xx+yy) ≤ n && (k % 12 == 1 || k % 12 == 5) && (s[k] = !s[k])
(k -= xx ) ≤ n && (k % 12 == 7 ) && (s[k] = !s[k])
(k -= 2yy); 1 ≤ k ≤ n && x > y && (k % 12 == 11) && (s[k] = !s[k]) Even if it weren't faster! Please consider this 🙏, thanks! |
I'm not sure why there are so many 1 character names, I find them very difficult to deal with, both visually and trying to find things with the editor. Except for i,j,k in for loops, I believe in having a minimum of 3 characters, and hopefully not a substring of something else in the function. |
I'm not sure I like though the |
It's hard to meaningfully name numbers. |
Yes, that's true, but I still think avoiding visual confusion can be useful. |
@ScottPJones I'm trying to test this in an objective way; by showing the code to my classmates (Java is all they have seen mostly), and also to the students at other careers in order to have their non technical opinions. We all agree that since it's year 2015, it's ok to use unicode if we can, I would understand if the Isn't this why we have unicode for identifiers? IMHO I want Julia to go forward into the future in every aspect not backwards! (that's my greed) The old languages should be the ones trying to catch up (I don't care about them) and new ones too ie, Swift. I think that the prevalence of short identifiers is in part because of this, some like: You seem to think a lot about the dogmas, status quo and paradigms of the current and past generations of programmers, why not think in the future ones also? There is no spoon! ✨ |
For what it's worth, I'm fine with the |
In particular, you can infer from just the local code that it is a variable name, not an operation, since it gets assigned to (although technically, it could be a method definition, but then using it without calling it would be pretty weird). |
@Ismael-VC I'm not against the use of Unicode in julia, it was simply that to me, I do like to think about future paradigms, that's why I'm so into julia now. I think it really has quite a lot going for it. About inferring from the local code, yes, but in practice, that all depends on how many pages of local code you have (and the short names make it hard to find all the places a particular variable is used) |
@ScottPJones, I used it because that Perhaps in this cases a comment would be nice?
|
I like the spirit of the Quorum Language, The world's first evidence-oriented programming language:
|
Yes, in that example, it's small enough, I'm not sure we'd need to have comments all over if this is going to be standard julia practice, but I do think that square and square root not being operators, but rather parts of variable names, should at the very least be well documented, especially in the "Noteworthy differences" sections. Once you learn that they are part of identifiers and not operators, you can survive... (I still wonder why somebody didn't make them operators, as that seems to be more the usually julian practice) |
@Ismael-VC I believe that the mstr(n) in the pseudo code is an array of length 46189. It is not really clear. |
I just looked the square and square root symbols up, and I think it may be a mistake that they are allowed to be used for general variable names. |
@ScottPJones julia> √4
2.0
julia> p√ = 3
ERROR: syntax: extra token "√" after end of expression Names like function lj(x, A, B)
x⁻² = x^-2
x⁻⁶ = (x⁻²)^3
x⁻¹² = (x⁻⁶)^2
A*x⁻¹² - B*x⁻⁶
end to ensure that intermediate products are reused optimally in time-critical code. Computations like these are often very difficult for compilers to optimize to the level that humans can. |
Trying to use julia> √a = 4
sqrt (generic function with 13 methods) |
@jiahao that's a good one, nice! |
sieve[n] = !sieve[n] | ||
end | ||
n = 3x² - y² | ||
if x > y && n ≤ limit && n % 12 == 11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test if this line gives a BoundsError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pabloferz the test passed as is, only build 1.0.6386 failed but it's not related I think ...putting the @inbounds
back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I wasn't really paying attention to the x > y
condition in there (which guaranties n is not negative). So the index will be always in bounds given the conditionals. In the case of the previous implementation is was really necessary.
@jiahao I got confused because both |
@jiahao The names used have nothing at all to do with whether you cache something or not. |
LGTM now! One of the AppVeyor tests timed out. But I think this is OK otherwise. |
throw(ArgumentError("requested number of primes must be ≤ $(typemax(Int)), got $limit")) | ||
|
||
"Returns a collection of the prime numbers ≤ `limit`." | ||
primes(limit::Union{Integer,AbstractVector{Bool}}) = find(primesmask(limit)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the function accepts either an Integer (a limit) or an array of Bool
s to mask so I think maybe n
is better than limit
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pabloferz I we're going to export primesmask
I think we wouldn't need the method:
primes(limit::Union{Integer,AbstractVector{Bool}})
Since there is already: primesmask(sieve::AbstractVector{Bool})
. It could end up like this:
primes(limit::Integer)
primesmask(sieve::AbstractVector{Bool})
Which makes the interface better IMHO and easier to document. So that the people that find primesmask(sieve::AbstractVector{Bool})
useful, can explicitly use it instead. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also imply removing:
primesmask(limit::Int) = primesmask(falses(limit))
And:
primesmask(limit::Integer) = limit <= typemax(Int) ? primesmask(Int(limit)) :
throw(ArgumentError("requested number of primes must be ≤ $(typemax(Int)), got $limit"))
In order to move that logic into primes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with those changes. But I guess we need someone else to agree (@StefanKarpinski for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this:
"Returns a collection of the prime numbers ≤ `limit`."
primes(limit::Integer) = limit <= typemax(Int) ? find(primesmask(falses(limit))) :
throw(ArgumentError("requested number of primes must be ≤ $(typemax(Int)), got $limit"))
@pabloferz If you or anyone else have some time to look at my attempt to understand the pseudocode you posted: And give me a hint I would be very grateful! Obviously I'm lost. If I run it, it gets stuck at the fourth phase. 😟 |
@Ismael-VC I guess can check http://stackoverflow.com/questions/19388106/the-sieve-of-atkin/20572948#20572948 to get more insight on the algorithm. But in the long run I guess it is better to implement an optimized Sieve of Eratosthenes instead (I'm trying to do that). |
@pabloferz the pseudocode you posted is for an Eratosthenes sieve not an Atkin one if I understand correctly:
I also understand that Atkin's sieve is faster for larger primes |
Is primesieve BSD 2-Clause License not compatible with MIT License? I'm reading about it but I'm not sure if I understand quite right: These links may serve as a better reference:
|
I wish GitHub could let me edit several files from the online interface and commit them all at once instead of forcing me to commit one by one. Is there a way to cancel unwanted CI tests from my side? 😟 |
This is the way to do it. I cancelled one of them and apparently someone else did the other three at the same time. |
@yuyichao thanks! |
This LGTM, AppVeyor build seems to have timed out. |
@Ismael-VC, can you check my code for prime generation in the thread #11594? It is very fast. Here are few results: |
@ironman353 yes I will! Is this the one you ported form Java that you where mentioning? I'll test each and every implementation in #11594 and try to implement something that reflects the advantages of all this codes in hope it's faster than @aiorla @danaj @VicDrastik @pabloferz @ironman353 By the way which is a good point of comparison? I'm using |
@Ismael-VC I'd be quite surprised if you passed primesieve. If you're 1/2 to 1/4 its speed you're doing very well. This is generating and counting primes -- of course there are much faster ways to just count. For sieving, it's the fastest out there (yafu beats it by a few percent in some ranges). You'll want a segmented sieve for controlling memory and for performance if you want to compare times for large sizes. For reference, primesieve running single threaded: 0.12s 10^9 These use very little memory. This is in C rather than Julia, but that gives you some idea of the times. yafu is relatively similar in time. Perl/ntheory is about 1/2 the speed. primegen (Bernstein's segmented Sieve of Atkin) is about 1.5x the speed up to 10^11, then gets relatively slower (Perl/ntheory passes it at 10^12 and is almost 2x faster at 10^13). |
@Ismael-VC, yes I converted my code in JAVA to Julia. It is in that thread. |
Wow, I didn't remember the primes issue! @ironman353 results were much better than master, I think they are worth a try. (although it's Eratosthenes and this PR seems to be more Atkin "oriented") |
Now that #12025 is ready, I believe we can close this in favour of that one. |
Closing in favor of #12025 |
There was a lot of interest to improve the current
Base.primes
function in #11594, but no PRs, I hope this gets the ball rolling even more.References:
atkin
vsBase.primes
IJulia notebook.More complete test using JuliaBox and Julia version
0.4.0-dev+5491
:sieves.jl
test.