HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Filtering function

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
functionfilteringstackoverflow

Problem

#include 
#include 
#include 
#include 
#include 
#include 
#include 

using namespace std;

ostream* outfile = &cout;

bool testCantor(unsigned long long p, unsigned long long q){
    while(q % 3 == 0){
        q /= 3;
        if (p/q == 1) return p==q;
        p %= q;
    }
    unsigned long long p_start = p;
    do{
        unsigned long long p3 = p * 3;
        if(p3/q == 1) return false;
        p = p3 % q;
    } while(p != p_start);
    return true;
}

void genRational(unsigned long long minNum, unsigned long long maxNum, int step, string *result){
    ostringstream buffer;
    for(unsigned long long q = minNum; q  usedThreads;
    vector results;
    for(int i = 0; i join();
    for(auto i = results.begin(); i != results.end(); i++) *outfile << **i ;
    return 0;
}


That is a program I am currently using to get an idea of rational numbers on the Cantor Set. I'm looking for any way to optimize (currently running from 313 to 316 with 320 threads (one per core) takes 5 hours and it will probably take 819 times longer to compute from 316 to 319).

Is there any way to further optimize this?

Solution

It's probably not going to speed it up a huge amount, but you are doing un-necessary dynamic allocations for thread and string (and leaking memory in the process, as these are never deleted).

void genRational(unsigned long long minNum, unsigned long long maxNum, int step, string *result)


Just return the string by value. Named return value optimization should kick in (and could give you a slight performance gain here, because you don't need to dynamically allocate a std::string each time). I assume you're doing this as you're unsure how to get returned values from a thread. The answer is to use either a std::packaged_task or std::async, both of which give a std::future:

std::string genRational(unsigned long long minNum, unsigned long long maxNum, int step)
{
     ...
}

std::vector> futures;
std::vector results;

for(int i = 0; i < THREADS; i++){
    futures.emplace_back(std::async(std::launch_async, genRational, minNum + i, maxNum, THREADS));
}

for(auto& f : futures) {
    results.push_back(f.get());
}


For further optimization, I'd profile and see exactly where the hotspots are (which will be in one of your testCantor and genRational functions). Look at the most expensive operations there (which will almost certainly be % and /). If you can find a way of reducing the number of these and/or coming up with something that uses other primitive operators in their place, you may be able to get a performance improvement - but I doubt you'll be able to squeeze out anything that'll make the running time reasonable for numbers as large as you want.

Code Snippets

void genRational(unsigned long long minNum, unsigned long long maxNum, int step, string *result)
std::string genRational(unsigned long long minNum, unsigned long long maxNum, int step)
{
     ...
}

std::vector<std::future<std::string>> futures;
std::vector<std::string> results;

for(int i = 0; i < THREADS; i++){
    futures.emplace_back(std::async(std::launch_async, genRational, minNum + i, maxNum, THREADS));
}

for(auto& f : futures) {
    results.push_back(f.get());
}

Context

StackExchange Code Review Q#42394, answer score: 2

Revisions (0)

No revisions yet.