sieve or eratosthenes calculator -- running into memory issues and crashing with numbers >=1,000,000

Question

I'm not exactly sure why this is. I tried changing the variables to long long, and I even tried doing a few other things -- but its either about the inefficiency of my code (it literally does the whole process of finding all primes up to the number, then checking against the number to see if its divisible by that prime -- very inefficient, but its my first attempt at this and I feel pretty accomplished having it work at all....)

Or the fact that it overflows the stack. Im not sure where it is exactly, but all I know is that it MUST be related to memory and the way its dealing with the number.

If I had to guess, Id say its a memory issue happening when it is dealing with the prime number generation up to that number -- thats where it dies even if I remove the check against the input number.

I'll post my code -- just be aware, I didnt change long long back to int in a few places, and I also have a SquareRoot Variable that is not used, because it was supposed to try and help memory efficiency but was not effective the way I tried to do it. I Just never deleted it. I will clean up the code when and if I can successfully finish it.

As far as I am aware though, it DOES work pretty reliably for 999,999 and down, I actually checked it up against other calculators of the same type and it seemingly does generate the proper answers.

If anyone can help or explain what I screwed up here, your helping a guy trying to learn on his own without any school or anything. so its appreciated.

#include <iostream>
#include <cmath>

void sieve(int ubound, int primes[]);

int main()
{
    long long n;
    int i;
    std::cout << "Input Number: ";
    std::cin >> n;

    if (n < 2) {
        return 1;
    }

    long long upperbound = n;
    int A[upperbound];
    int SquareRoot = sqrt(upperbound);
    sieve(upperbound, A);

    for (i = 0; i < upperbound; i++) {
        if (A[i] == 1 && upperbound % i == 0) {
            std::cout << " " << i << " ";
        }
    }
    return 0;
}

void sieve(int ubound, int primes[])
{
    long long i, j, m;

    for (i = 0; i < ubound; i++) {
        primes[i] = 1;
    }

    primes[0] = 0, primes[1] = 0;

    for (i = 2; i < ubound; i++) {
        for(j = i * i; j < ubound; j += i) {
            primes[j] = 0;
        }
    }
}

Answer 1

If you used legal C++ constructs instead of non-standard variable length arrays, your code will run (whether it produces the correct answers is another question).

The issue is more than likely that you're exceeding the limits of the stack when you declare arrays with a million or more elements.

Therefore instead of this:

long long upperbound = n;
A[upperbound];

Use std::vector:

#include <vector>
//...
long long upperbound = n;
std::vector<int> A(upperbound);

and then:

sieve(upperbound, A.data());

The std::vector does not use the stack space to allocate its elements (unless you have written an allocator for it that uses the stack).

As a matter of fact, you don't even need to pass upperbound to sieve, as a std::vector knows its own size by calling the size() member function. But I leave that as an exercise.

Live example using 2,000,000

Answer 2

First of all, read and apply PaulMcKenzie's advice. That's the most important thing. I'm only addressing some teeny bits of your question that remained open.

It seems that you are trying to factor the number that you misleadingly called upperbound. The mysterious role of the square root of this number is related to this fact: if the number is composite at all - and hence can be computed as the product of some prime factors - then the smallest of these prime factors cannot be greater than the square root of the number. In fact, only one factor can possibly be greater, all others cannot exceed the square root.

However, in its present form your code cannot draw advantage from this fact. The trial division loop as it stands now has to run up to number_to_be_factored / 2 in order not to miss any factors because its body looks like this:

if (sieve[i] == 1 && number_to_be_factored % i == 0) {
   std::cout << " " << i << " ";
}

You can factor much more efficiently if you refactor your code a bit: when you have found the smallest prime factor p of your number then the remaining factors to be found must be precisely those of rest = number_to_be_factored / p (or n = n / p, if you will), and none of the remaining factors can be smaller than p. However, don't forget that p might occur more than once as a factor.

During any round of the proceedings you only need to consider the prime factors between p and the square root of the current number; if none of those primes divides the current number then it must be prime. To test whether p exceeds the square root of some number n you can use if (p * p > n), which is computationally more efficient that actually computing the square root.

Hence the square root occurs in two different roles:

• the square root of the number to be factored limits the amount of sieving that needs to be done
• during the trial division loop, the square root of the current number gives an upper bound for the highest prime factor that you need to consider

That's two faces of the same coin but two different usages in the actual code.

Note: once you got your code working by applying PaulMcKenzie's advice, you might also to consider posting over on Code Review.