Word frequency program - file input too large?

Question

I'm still working on the problem mentioned in this post: Sorting vector of strings with leading numbers

The original problem is as follows:

Write a complete C++ program that outputs the k most frequently used words in file input.txt, one per line in descending order of frequency, where k is a nonnegative integer read from input. Ties are broken arbitrarily, and if there are only u different words in input.txt and u < k, then the output has only u entries. For this problem, you may not use any STL class or algorithm except vector and string. A word is a maximal block of non-white-space characters with punctuations removed. Each output line consists of a word followed by its frequency count. (inputs and k-values are given)

Thanks to those who suggested using a struct, I ended up with a little bit more efficient solution with less code.

However, the problem is, for text files that are relatively large (consisted of >400000 words), my program can keep running for more than 5 minutes and gives no result whatsoever. The program runs perfectly on small file inputs. I'm not sure whether it's because the file was too big, or there's a problem with the algorithm itself that causes memory overflow/corruption.

Here's my code for the program:

struct word_freq {
int freq;
string word;
};

bool operator<(const word_freq& a, const word_freq& b) {
    return a.freq < b.freq;
}
void word_frequencies(ifstream& inf, int k)
{
vector <string> input;
string w;
while (inf >> w)
{
    remove_punc(w);
    input.push_back(w);
}
sort(input.begin(), input.end());

// initialize frequency vector
vector <int> freq;
for (size_t i = 0; i < input.size(); ++i) freq.push_back(1);

// count actual frequencies
int count = 0;
for (size_t i = 0; i < input.size()-1; ++i)
{
    if (input[i] == input[i+1])
    {
        ++count;
    } else
    {
        freq[i] += count;
        count = 0;
    }
}

// words+frequencies
vector <word_freq> wf;
for (int i = 0; i < freq.size(); ++i)
{
    if (freq[i] > 1 || is_unique(input, input[i]))
    {
        word_freq st = {freq[i], input[i]};
        wf.push_back(st);
    }
}

// printing
sort(wf.begin(), wf.end());
if (wf.size() < k)
{
    for (int i = wf.size()-1; i >= 0; --i)
    {
        cout << wf[i].word << " " << wf[i].freq << endl;
    }
} else
{
    for (int i = wf.size()-1; i >= wf.size()-1-k; --i)
    {
        cout << wf[i].word << " " << wf[i].freq << endl;
    }
}
}

If anyone can point out mistakes made, it would be greatly appreciated.

Answer 1

If you use reserve(int) after allocating your vectors, performance will be much much better.

Pushing back to vectors constantly causes memory fragmentation.

The reason is that vectors are constantly outgrowing their allocated boundaries, and get reallocated often. Reallocating small objects often is expensive and causes direct impact on performance.

Calling reserve with a large enough chunk of memory initially, and calling it again when the vector's size matches its capacity, helps avoid this issue.

More here:

What is memory fragmentation?

And here:

Should I worry about memory fragmentation with std::vector?

Small demonstration with performance measurements:

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

int main()
{
        std::vector<std::string> slow;
        std::string d = "divide and conquer";

        std::chrono::time_point<std::chrono::system_clock> start, end;
        start = std::chrono::system_clock::now();

        // I get reallocated all the time
        for ( int i=0; i < 100000; i++ )
        {
            slow.push_back(d);
        }

        end = std::chrono::system_clock::now();

        std::chrono::duration<double> elapsed_seconds = end-start;
        std::time_t end_time = std::chrono::system_clock::to_time_t(end);

        std::cout << "elapsed time v1: " << elapsed_seconds.count() << "s\n";

        start = std::chrono::system_clock::now();

        //I don't move around
        slow.reserve(100000);
        slow.clear();
        for ( int i=0; i < 100000; i++ )
        {
            slow.push_back(d);
        }

        end = std::chrono::system_clock::now();

        elapsed_seconds = end-start;
        end_time = std::chrono::system_clock::to_time_t(end);

        std::cout << "elapsed time v2: " << elapsed_seconds.count() << "s\n";
        return 0;
}

Output:

    elapsed time v1: 0.014085s

    elapsed time v2: 0.004597s

Answer 2

You make your program to do way too match by memory and calculations. First you read all words into memory and sort them. Then you calculate frequencies and populate yet another vector. You should have std::vector<word_freq> on the first place, keep it sorted by word (by inserting elements into proper place) and insert new element or increase counter on existing one. Then resort this vector by frequencies and print.

For example how you could rewrite your loop:

struct word_freq {
    int freq;
    std::string word;

    word_freq( const std::string &w ) : word( w ), freq( 0 ) {}
};


void addWord( std::vector<word_freq> &v, const std::string &word )
{
     word_freq tmp( word );
     auto p = std::equal_range( v.begin(), v.end(), tmp, 
         []( const word_freq &w1, const word_freq &w2 ) {
             return w1.word < w2.word;
     } );
     if( p.first == p.second )  // not found
         p.first = v.insert( p.second, tmp ); // insert into proper place
     p.first->freq++; // increase freq counter
}

// ......
std::vector<word_freq> words;
string w;
while (inf >> w)
{
    remove_punc(w);
    addWord( words, w );
}
// here your vector sorted by words, there are no dups and counters have proper value already
// just resort it by freq and print

Details on how keep vector sorted can be found here how do you insert the value in a sorted vector?

On another side keep std::vector<word_freq> sorted will require way too match inserts into middle or beginning of the vector, which could be quite expensive and slow. So if you implement described logic and make it work on small examples and it is still too slow for your big input - you should sort vector of indexes instead of vector of word_freq itself. That will still require to insert to begining or middle of vector of integers, but such operation is significantly cheaper and faster. Details on how sort indexes instead of vector itself can be found here: compare function of sort in c++ for index sort