My Sieve of Eratosthenes takes too long

Question

I have implemented "sieve of eratosthenes" to solve an SPOJ problem. Though the output is fine, my submission exceeds the time limit. How can I reduce the run time?

int main()
{
  vector<int> prime_list;
  prime_list.push_back(2);
  vector<int>::iterator c;
  bool flag=true;
  unsigned int m,n;
  for(int i=3; i<=32000;i+=2)
  {
    flag=true;
    float s = sqrt(static_cast<float>(i));
    for(c=prime_list.begin();c<=prime_list.end();c++)
    {
        if(*c>s)
            break;
        if(i%(*c)==0)
        {
            flag=false;
            break;
        }
    }
    if(flag==true)
    {
        prime_list.push_back(i);
    }
  }
  int t;
  cin>>t;
  for (int times = 0; times < t; times++)
  {
    cin>> m >> n;
    if (t) cout << endl;
    if (m < 2)
        m=2;
    unsigned int j;
    vector<unsigned int> req_list;
    for(j=m;j<=n;j++)
    {
        req_list.push_back(j);
    }
    vector<unsigned int>::iterator k;
    flag=true;
    int p=0;
    for(j=m;j<=n;j++)
    {
        flag=true;
        float s = sqrt(static_cast<float>(j));
        for(c=prime_list.begin();c<=prime_list.end();c++)
        {
            if((*c)!=j)
            {
                if((*c)>s)
                    break;
                if(j%(*c)==0)
                {
                    flag=false;
                    break;
                }
            }
        }
        if(flag==false)
        {
            req_list.erase (req_list.begin()+p);
            p--;
        }
        p++;
    }
    for(k=req_list.begin();k<req_list.end();k++)
    {
        cout<<*k;
        cout<<endl;
    }
  }
}

Answer 1

Profile your code, find hotspots, eliminate them. Windows, Linux profiler links.

Answer 2

I think one way to slightly speed up your sieve is the prevention of using the mod operator in this line.

if(i%(*c)==0)

Instead of the (relatively) expensive mod operation, maybe if you iterated forward in your sieve with addition.

Honestly, I don't know if this is correct. Your code is difficult to read without comments and with single letter variable names.

Answer 3

I'd throw out what you have and start over with a really simple implementation of a sieve, and only add more complexity if really needed. Here's a possible starting point:

#include <vector>
#include <iostream>

int main() {
    int number = 32000;
    std::vector<bool> sieve(number,false);
    sieve[0] = true;  // Not used for now, 
    sieve[1] = true;  //   but you'll probably need these later.

    for(int i = 2; i<number; i++) {
        if(!sieve[i]) {
            std::cout << "\t" << i;
            for (int temp = 2*i; temp<number; temp += i)
                sieve[temp] = true;
        }
    }
    return 0;
}

For the given range (up to 32000), this runs in well under a second (with output directed to a file -- to the screen it'll generally be slower). It's up to you from there though...

Answer 4

Your code is slow because you did not implement the Sieve of Eratosthenes algorithm. The algorithm works that way:

1) Create an array with size n-1, representing the numbers 2 to n, filling it with boolean values true (true means that the number is prime; do not forget we start counting from number 2 i.e. array[0] is the number 2)
2) Initialize array[0] = false.
3) Current_number = 2;
3) Iterate through the array by increasing the index by Current_number.
4) Search for the first number (except index 0) with true value.
5) Current_number = index + 2;
6) Continue steps 3-5 until search is finished.

This algorithm takes O(nloglogn) time. What you do actually takes alot more time (O(n^2)). Btw in the second step (where you search for prime numbers between n and m) you do not have to check if those numbers are prime again, ideally you will have calculated them in the first phase of the algorithm.

As I see in the site you linked the main problem is that you can't actually create an array with size n-1, because the maximum number n is 10^9, causing memory problems if you do it with this naive way. This problem is yours :)

Answer 5

I am not really sure that you have implemented the sieve of Erasthotenes. Anyway a couple of things that could speed up to some extent your algorithm would be: Avoid multiple rellocations of the vector contents by preallocating space (lookup std::vector<>::reserve). The operation sqrt is expensive, and you can probably avoid it altogether by modifying the tests (stop when the x*x > y instead of checking x < sqrt(y).

Then again, you will get a much better improvement by revising the actual algorithm. From a cursory look it seems as if you are iterating over all candidates and for each one of them, trying to divide with all the known primes that could be factors. The sieve of Erasthotenes takes a single prime and discards all multiples of that prime in a single pass.

Note that the sieve does not perform any operation to test whether a number is prime, if it was not discarded before then it is a prime. Each not prime number is visited only once for each unique factor. Your algorithm on the other hand is processing every number many times (against the existing primes)

Answer 6

The way I understand the problem is that you have to generate all primes in a range [m,n].

A way to do this without having to compute all primes from [0,n], because this is most likely what's slowing you down, is to first generate all the primes in the range [0,sqrt(n)].

Then use the result to sieve in the range [m,n]. To generate the initial list of primes, implement a basic version of the sieve of Eratosthenes (Pretty much just a naive implementation from the pseudo code in the Wikipedia article will do the trick).

This should enable you to solve the problem in very little time.

Here's a simple sample implementation of the sieve of Eratosthenes:

std::vector<unsigned> sieve( unsigned n ) {
    std::vector<bool> v( limit, true ); //Will be used for testing numbers
    std::vector<unsigned> p;            //Will hold the prime numbers

    for( unsigned i = 2; i < n; ++i ) {
        if( v[i] ) {                    //Found a prime number
            p.push_back(i);             //Stuff it into our list

            for( unsigned j = i + i; j < n; j += i ) {
                v[i] = false;           //Isn't a prime/Is composite
            }
        }
    }

    return v;
}

It returns a vector containing only the primes from 0 to n. Then you can use this to implement the method I mentioned. Now, I won't provide the implementation for you, but, you basically have to do the same thing as in the sieve of Eratosthenes, but instead of using all integers [2,n], you just use the result you found. Not sure if this is giving away too much?

Answer 7

Since the SPOJ problem in the original question doesn't specify that it has to be solved with the Sieve of Eratosthenes, here's an alternative solution based on this article. On my six year old laptop it runs in about 15 ms for the worst single test case (n-m=100,000).

#include <set>
#include <iostream>

using namespace std;

int gcd(int a, int b) {
   while (true) {
      a = a % b;

      if(a == 0)
         return b;

      b = b % a;

      if(b == 0)
         return a;
   }
}

/**
 * Here is Rowland's formula. We define a(1) = 7, and for n >= 2 we set 
 *
 * a(n) = a(n-1) + gcd(n,a(n-1)). 
 *
 * Here "gcd" means the greatest common divisor. So, for example, we find
 * a(2) = a(1) + gcd(2,7) = 8. The prime generator is then a(n) - a(n-1),
 * the so-called first differences of the original sequence.
 */
void find_primes(int start, int end, set<int>* primes) {
   int an;        // a(n)
   int anm1 = 7;  // a(n-1)
   int diff;

   for (int n = start; n < end; n++) {
      an = anm1 + gcd(n, anm1);

      diff = an - anm1;

      if (diff > 1)
         primes->insert(diff);

      anm1 = an;
   }
}

int main() {
   const int end = 100000;
   const int start = 2;

   set<int> primes;

   find_primes(start, end, &primes);
   ticks = GetTickCount() - ticks;

   cout << "Found " << primes.size() << " primes:" << endl;

   set<int>::iterator iter = primes.begin();
   for (; iter != primes.end(); ++iter)
      cout << *iter << endl;
}