5
\$\begingroup\$

I'm trying to read a file containing over 10.000 lines and each line contains an ID for an individual and a very long string composed of more than 1 million characters (numbers as example: 2200500200...205 until 1 million characters long).

I need to go through this long string and retrieve a subset based on flag information formed by 0 or 1s coming from a string called flagArray01String in the sample code, so I'm checking one string of the same length and printing the subset in a file.

I'm in doubt if I should convert the long strings (string from file and string composed of flags) to char and perform the loop after that. I'm looking forward to receiving any comments that could help me obtaining a better performance.

Should I load all the strings in memory for some individuals? Are there any other performance tricks?


Example of a sample file:

ID1 2020205000200200202020
ID2 2020205000200200202020
ID3 2020205000200200202020

...
ID10000 2020205000200200202020

Example of flag: flag = 1000001111111111111100, where all the positions with 1´s should be retrieved and printed in a separated file.


if (IN_file.is_open()) 
{
    getline(IN_file, headerFileOriginal);
    while (IN_file >> idOriginal >> longString) 
    {
        sizeTemp = longString.size();
        if (sizeTemp > 0)
            numbIdsOriginal++;
        for (int p = 0; p < flagArray01String.size(); p++) 
        {
            if ((flagArray01String[p] == '1')) 
            {
                subsetString = subsetString + longString[p];
            }
        }
        OUT_FILE << idOriginal << '\t' << subsetString << endl;
        subsetString = "";
    }
} 
else 
{
    myfileLog << "Problems to open file " << IN_file_StringName << endl;
    cout << "Problems to open file " << IN_file_StringName << endl;
}

IN_file.close();
OUT_FILE.flush();
OUT_FILE.close();
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review. Please don't forget to delete your original question at Stack Overflow after moving it here. It's not good to have the same question running at more than 1 SE network site. \$\endgroup\$ – πάντα ῥεῖ Jul 6 '18 at 17:49
  • \$\begingroup\$ Just to clarify, your input data is space separated and your output data is tab separated. That there was no loss in formatting when you copied your code to CR. \$\endgroup\$ – Snowhawk Jul 7 '18 at 6:04
  • \$\begingroup\$ Hi Snowhawk, yes, input is space and output tab. Thanks! \$\endgroup\$ – Adroacir Jul 7 '18 at 11:18
7
\$\begingroup\$

I see some things that may help you improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It's not explicitly in your question, but the lack of std:: in the code leads me to suspect it.

Use consistent variable naming

We have variables named OUT_FILE, subsetString, IN_file_StringName with no discernable consistent style. Pick a form, say, camel case such as subsetString and use it consistently. Also, all caps names such as OUT_FILE should only be used for macros (and you probably don't really need macros much with modern C++).

Provide a complete interface

This is partly to do with getting good reviews, but also has a lot to do with good design. The code as posted leaves a lot out. It's not a complete function, and so reviewers have to guess as to what the missing variables are. So in attempting to turn the code into a function which could be tested, I encapsulated it like this:

int original(std::ifstream &IN_file, std::ofstream &OUT_FILE, 
            const std::string &IN_file_StringName, const std::string &flagArray01String) 
{
    std::string headerFileOriginal;
    std::string idOriginal;
    std::string longString;
    int numbIdsOriginal{0};
    std::string subsetString;

    // original code here

    return numbIdsOriginal;
}

I actually deleted the line containing myfileLog for reasons I'll describe later. Note that there are a number of variables passed in and a number which can be local to the function.

Don't store what you don't need

Because we don't have the full context, it's not clear if headerFileOriginal is ever used or is just discarded. If the latter, I'd suggest this instead:

IN_file.ignore(std::numeric_limits<std::streamsize>::max(), '\n'); 

Rethink error handling

As the code is currently written, the processing is done within a loop inside an is_open() call. I'd refactor that to pull the processing into a separate function and write it like this:

std::ifstream in{inputFileName};
if (!in) {
    // log errors and quit
}
processMask(in, out, maskString);

All opening/closing/flushing should be done outside the function that does the line processing.

Prefer std::istream to std::ifstream

One could concievably get input from any kind of input stream and not just a file. The same is true of output, so for that reason and to preserve maximum flexibility in the interface, I'd write the function with a prototype like this:

int processMask(std::istream &in, std::ostream &out, const std:string &maskString);

This allows for flexibility in input/output and also, by declaring the maskString asconst` makes clear that it will not be altered by the function.

Use a standard function

The standard template library has a lot of handy stuff in it, much of which is highly optimized for performance. For that reason, it's often better to use an appropriate STL function rather than writing your own. In this case, I'd suggest using std::copy_if. Here's how a loop might be implemented using this:

while (in >> id >> longString) 
{
    out << id << '\t';
    ++count;
    auto it{flagstring.cbegin()};
    auto end{flagstring.cend()};
    std::copy_if(longString.cbegin(), longString.cend(), 
        std::ostream_iterator<char>(out),
        [&it,&end](const char){
            return it != end && *it++ == '1';
        });
    out << '\n';
}

This works by using a lambda as a predicate function. It has the effect of copying each character from the input to the output as long as the condition is true, and the condition simply iterates through the flag string.

Don't use std::endl if you don't really need it

The difference between std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Don't explicitly flush the stream unless needed

When a std::ofstream iterator goes out of scope, the file is automatically flushed and closed by the destructor. So for that reason, rather than explicitly calling flush() or close(), I'd recommend simply limiting the scope of the variable to only what is needed and rely on the destructor to do all of that.

Understand the limits of performance

I strongly suspect this program is I/O bound. That is, I think it's very likely that almost all of the full duration required by this program is not in processing the data, but in simply reading and writing from files. One simple test is to compare a simple line-at-a-time C++ copy function with the whole program. The difference between the running of those two is likely to be an approximation of the maximum time you can trim by optimizing the algorithm. I created a sample file with 10,000 data lines, each of which has a little over one million characters in the second field. It takes about 1:03 (minutes:seconds) on my machine to process this 9.4 Gb file no matter what I do with for processing.

\$\endgroup\$
3
\$\begingroup\$

Minimize Reading

For the moment I'm going to assume that the long lines in the file are always exactly 1 million characters long (or some length we know ahead of time in any case). I'm also going to assume that the flag string is quite a bit shorter than that--the one you show is 22 characters long, and the last 2 of those are zeros (which, from our perspective can be sort of ignored).

So, we start by trimming any trailing zeros from the flag string.

Then, instead of reading entire lines from the input file, we read the ID, and the first N characters of the long string (where N = length of the flag string).

We then copy characters from that string to the output where the flag has a 1.

Then (important part) we do a seek from the current position to the beginning of the next line.

Avoid std::endl

Your code to write the data to the output file looks like this:

OUT_FILE << idOriginal << '\t' << subsetString << endl;

std::endl not only adds a new-line to the string, but also flushes the output file's buffer. This will typically result in relatively slow writing. Unless you truly need to ensure that each line is flushed as it's written, you can get roughly the same result (but usually quite a bit more quickly) by just writing a new-line character:

OUT_FILE << idOriginal << '\t' << subsetString << '\n';
\$\endgroup\$
  • \$\begingroup\$ Dear Jerry Coffin, thanks a lot for your comments. Im learning a lot from you and Edward. Only one comment regarding the assumptions, I forgot to mention that in the scenario considering 1 million values in the string, more than 70% must be subselected and 30% that will be avoided will likelly be randomly positioned, so the example containing several positions with the same content was not realistic from my side. Sorry for this mistake. Do you have any suggestions regarding this new added information? Many thanks for your attention in advance. \$\endgroup\$ – Adroacir Jul 6 '18 at 22:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.