Why is my Ruby code for Project Euler #10 so slow?

Question

I am relatively new to Ruby but it seems simple enough as far as a language goes. I am working through the Euler Project with Ruby and I'm having a huge issue with speed on the following:

The sum of the primes below 10 is 2 + 3 + 5 + 7 = 17. Find the sum of all the primes below two million.

My code:

beginning_time = Time.now
(1..10000).each { |i| i }

def isPrime(num)
    factors = 0
    primecount = 1
    while primecount <= num
        if (num%primecount == 0)
            factors += 1
        end
        if (factors > 2)
            return false
        end
        primecount += 1
    end
    return true 
end

def make_sieve(num)
    sieve = Array.new(num)
    summation = 0
    for i in 1..num
            if(isPrime(i) == true)
                summation += i
                puts i
                for x in i..num
                    if x%i == 0
                    # Go through entire array and make all multiples of i False
                        sieve[x] = false
                    else
                        sieve[i] = true                     
                    end
                end
            else
                # If i is NOT prime, move to the next number. in the For Loop
                next
            end
    end
    puts summation
end

make_sieve(2000000)


end_time = Time.now
puts "Time elapsed #{(end_time - beginning_time)*1000} milliseconds"

I think I have the right idea with the sieve but I really have no clue what's going on that makes this program run so slow. I run it with 20,000 and it takes about 15 seconds, which seems slow even still, although the output comes out MUCH faster than when I put 2,000,000.

Am I going about this the wrong way logically or syntactically or both?

Answer 1

Your isPrime() test is very slow on primes; but you don't even need it. The key to sieve is, initially all the numbers are marked as prime; then for each prime we mark off all its multiples. So when we get to a certain entry in the sieve, we already know whether it is a prime or not - whether it is marked true for being prime, or it is marked false for being composite (a multiple of some smaller prime).

There is no need to test it being prime, at all.

And to find the multiples, we just count: for 5, it's each 5th entry after it; for 7 - each 7th. No need to test them with % operator, just set to false right away. No need to set any of them to true, because all numbers were set to true at the start.

Answer 2

You seem to be writing JavaScript code in Ruby, and are missing the subtleties that makes Ruby so elegant. You should take a look at something like Ruby Best Practices, which is quite a light read but deals with using Ruby idioms instead of imposing the concepts of another language.

As has been said, the whole point of an Eratosthenes sieve is that you just remove all compound numbers from a list, leaving just the primes. There is no need to check each element for primeness.

This is a Rubyish solution. It runs in about 1.5 seconds. It is a little complicated by the representing number N by array element N-1, so (i+i+1 .. num).step(i+1) is equivalent to (n * 2 .. num).step(n)

def make_sieve(num)

  sieve = Array.new(num, true)

  sieve.each_with_index do |is_prime, i|
    next if i == 0 or not is_prime
    (i+i+1 .. num).step(i+1) { |i| sieve[i] = false }
  end

  puts sieve.each_index.select { |i| sieve[i] }.map { |i| i+1 }.inject(:+)

end

make_sieve(2_000_000)

output

142913828923