C++ Collatz Conjecture Optimization

Question

In ProjectEuler problem #14, one needs to find the longest Collatz chain, up to 1 million. I found a halfway decent way to do so, however, it feels like I'm just being stupid because I can't find a way to make this code more efficient (the code is supposed to only print out the solution, after it tests 1 to 1 million, but hasn't printed anyting out after 10 minutes). Am I tackling this problem the wrong way, or is there a way to optimize my existing code?

#include <iostream>
using namespace std;

int main()
{
    int i;
    int x;
    int n;
    int superN;
    int superI;

    superN = 0;
    superI = 0;

    for (i = 1; i <= 1000000; i++) {
        x = i;
        n = 1;

        do {
            if (x % 2 == 0) {
                x = x / 2;
            }

            if (x % 2 == 1 && x != 1) {
                x = 3 * x + 1;
            }

            n++;

            if (n > superN) {
                superN = n;
                superI = i;
            }
        } while (x != 1);
    }

    cout << "The number " << superI << " ran for " << superN << " terms.";
    system("pause");
    return 0;
}

Answer 1

You've got a few small problems:

1. I'm fairly sure that you are overflowing the int data type. Use a uint64_t to make this far less likely.
2. You should only update superI and superN outside of the while loop. This shouldn't matter, but it hurts performance.
3. On each iteration you should only modify x once. You currently might modify it twice, which might cause you to fall into an infinite loop. And your calculation of n will be off as well.
4. Use memoization to improve performance by caching old results.

Applying this, you could come up with some code like this:

#include <cstdint>
#include <iostream>
#include <map>
using namespace std;

int main()
{
    uint64_t i;
    uint64_t x;
    uint64_t n;
    uint64_t superN;
    uint64_t superI;

    std::map<uint64_t, uint64_t> memory;

    superN = 0;
    superI = 0;

    for (i = 1; i <= 1000000; i++) {
        x = i;
        n = 1;

        do {
            if (memory.find(x) != memory.end()) {
                n += memory[x];
                break;
            }

            if (x % 2 == 0) {
                x = x / 2;
            } else {
                x = 3 * x + 1;
            }

            n++;
        } while (x != 1);

        if (n > superN) {
            superN = n;
            superI = i;
        }

        memory[i] = n;
    }

    cout << "The number " << superI << " ran for " << superN << " terms.\n";
    system("pause");
    return 0;
}

Which takes 4 seconds to output:

The number 837799 ran for 556 terms.

Answer 2

I would suggest not to use memoization as for me it run slower; in my case (up to 10,000,000) the code below is faster without memoization. the main changes are:

1. only testing if the current number is even once (not need for an else-if).
2. using a bitwise operation instead of the modulo operation (slightly faster)

Apart from that I don't know why your code is so long (mine is below 200 milliseconds) maybe you compile as debug ?

bool isEven(uint64_t value)
{
    return (!(value & 1));
}

uint64_t solveCollatz(uint64_t start)
{
    uint64_t counter = 0;
    while (start != 1)
    {
        if(isEven(start))
        { 
            start /= 2;
        }
        else
        {
            start = (3 * start) + 1;
        }
        counter++;
    }

    return counter;
}

Answer 3

If you can use compiler intrinsics, particularly with counting and removing trailing zeros, you'll recognize you don't need to branch in the main loop, you'll always alternate odd and even. The memoization techniques that have been previously presented will rarely short-circuit around the math you're doing, since we're dealing with hailstone numbers - additionally, the majority of numbers only have one entry point, so if you see them once, you'll never see them again.

In GCC it'll look something like this:

#include <cstdint>
#include <iostream>
#include <unordered_map>
#include <map>
using namespace std;

using n_iPair = std::pair<uint32_t, uint64_t>;

auto custComp = [](n_iPair a, n_iPair b){
  return a.first < b.first;
};

int main()
{
    uint64_t x;
    uint64_t n;
    n_iPair Super = {0,0};

    for (auto i = 1; i <= 1000000; i++){
        x = i;
        n = 0;

        if (x % 2 == 0) {
          n += __builtin_ctz(x); // account for all evens
          x >>= __builtin_ctz(x); // always returns an odd
        }

         do{ //when we enter we're always working on an odd number

          x = 3 * x + 1; // always increases an odd to an even
          n += __builtin_ctz(x)+1; // account for both odd and even transfer
          x >>= __builtin_ctz(x); // always returns odd

        }while (x != 1);

        Super = max(Super, {n,i}, custComp);

    }

    cout << "The number " << Super.second << " ran for " << Super.first << " terms.\n";
    return 0;
}

Answer 4

If performance is critical, but memory isn't, you can use caching to improve the speed.

#include <iostream>
#include <chrono>
#include <vector>
#include <sstream>

std::pair<uint32_t, uint32_t> longestCollatz(std::vector<uint64_t> &cache)
{
    uint64_t length = 0;
    uint64_t number = 0;

    for (uint64_t current = 2; current < cache.size(); current++)
    {
        uint64_t collatz = current;
        uint64_t steps = 0;
        while (collatz != 1 && collatz >= current)
        {
            if (collatz % 2)
            {
                // if a number is odd, then ((collatz * 3) + 1) would result in
                // even number, but even number can have even or odd result,  so
                // we can combine two steps for even number, and increment twice.
                collatz = ((collatz * 3) + 1) / 2;
                steps += 2;
            }
            else
            {
                collatz = collatz / 2;
                steps++;
            }
        }

        cache[current] = steps + cache[collatz];

        if (cache[current] > length)
        {
            length = cache[current];
            number = current;
        }
    }
    return std::make_pair(number, length);
}

int main()
{
    auto start = std::chrono::high_resolution_clock::now();;

    uint64_t input = 1000000;
    std::vector<uint64_t> cache(input + 1);
    auto longest = longestCollatz(cache);

    auto end = std::chrono::high_resolution_clock::now();
    auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(end - start).count();
    std::cout << "Longest Collatz (index : value) --> " << longest.first << " : " << longest.second;
    std::cout << "\nExecution time: " << duration << " milliseconds\n";

    return EXIT_SUCCESS;
}