Reading from large binary files

Question

I am reading data out of some large (~2GB each) binary files; these have a well defined structure:

- Header (info about the next chunk of data)
- Chunk of data (any type)
- Repeat

I am using the following function to extract data from the files:

template<class T> std::vector<T> getFromBin(std::ifstream& ifs, int num);

template<class T> std::vector<T> getFromBin(std::ifstream& ifs, int num) {
    std::vector<T> output;
    output.reserve(num);

    T* buffer = new T[num]; 
    ifs.read((char*)buffer, sizeof(T) * num);

    for(int i = 0; i < num; ++i) {
        output.push_back(buffer[i]);
    }

    delete[] buffer;

    return output;
}

If I want to read e.g. 2 unsigned short int followed by 100000 unsigned int from the binary file, I can do:

std::ifstream ifs(file_name, std::ios::binary | std::ios::ate);
std::streampos total_bytes(ifs.tellg());
ifs.seekg(0, std::ios::beg);

std::vector<unsigned short int> h(getFromBin<unsigned short int>(ifs, 2));

std::vector<unsigned int> b(getFromBin<unsigned int>(ifs, 100000));

This works perfectly. It gives me the exact results I want, and I get them fast. Does the code above have any problems allocating/releasing memory? Are there any obvious ways I can make it run faster?

I have 1 problem: If I run my code in Valgrind, I get over 10000000 errors detected. This is the only function in my code that handles memory by hand, so I am confused.

To be clear, by code doesn't crash, it runs perfectly.

Message from Valgrind:

==8613== More than 10000000 total errors detected.  I'm not reporting any more.
==8613== Final error counts will be inaccurate.  Go fix your program!
==8613== Rerun with --error-limit=no to disable this cutoff.  Note
==8613== that errors may occur in your program without prior warning from
==8613== Valgrind, because errors are no longer being displayed.
==8613== 
==8613== Warning: client switching stacks?  SP change: 0x1ffe8ffa90 --> 0x1fff0007d0
==8613==          to suppress, use: --max-stackframe=7343424 or greater
==8613== 
==8613== HEAP SUMMARY:
==8613==     in use at exit: 0 bytes in 0 blocks
==8613==   total heap usage: 54,917,047 allocs, 54,917,047 frees, 5,703,921,853 bytes allocated
==8613== 
==8613== All heap blocks were freed -- no leaks are possible
==8613== 
==8613== For lists of detected and suppressed errors, rerun with: -s
==8613== ERROR SUMMARY: 10000000 errors from 92 contexts (suppressed: 0 from 0)

Answer 1

This works perfectly. It gives me the exact results I want…

This code only “works perfectly” on your particular machine. It is not portable.

Consider reading in a bunch of unsigned longs. The core of your code (pretending we’re only reading a single unsigned long and not a bunch of them) is basically this:

auto i = 0uL;
ifs.read(reinterpret_cast<char*>(&i), sizeof(unsigned long));

There are two problems here:

1. sizeof(unsigned long) is implementation-defined. I believe it is 4 on Windows; it’s 8 on Linux. The same data file will read differently on different machines.
2. The endianness of unsigned long is also implementation-defined. If the data in your file is big-endian, and your machine is big-endian, then (probably) no problem. But if the endianness is not the same, you’re going to get garbage.

There are also other issues you will run into if you try to read into more complex types. And “more complex” doesn’t mean all that complex. Just consider the humble int. Even if you get endianness right, there’s no guarantee that the sign bit will be in the same place across different implementations. Once you get into doubles and such, all bets are off.

Does the code above have any problems allocating/releasing memory?

Yes, because you are using naked new and delete. That is bad C++.

So long as you stick to simple types like unsigned int you will probably never have a problem. However, look at the structure of your code:

    T* buffer = new T[num]; 
    ifs.read((char*)buffer, sizeof(T) * num); // <---

    for(int i = 0; i < num; ++i) {
        output.push_back(buffer[i]);    // <---
    }

    delete[] buffer;

What happens if that call to push_back() throws? You may argue that it can’t, because you’ve already reserved the memory. But allocation failures are not the only way a push_back() can fail. Not only that, but ifs.read() might throw as well. If a throw happens, that buffer is leaked.

This is so easily fixed by just using a smart pointer:

    T* buffer = std::make_unique_for_overwrite<T[]>(num); // or std::unique_ptr<T[]>{new T[num]};
    ifs.read((char*)buffer.get(), sizeof(T) * num);

    for(int i = 0; i < num; ++i) {
        output.push_back(buffer[i]);
    }

    // buffer automatically deletes

Are there any obvious ways I can make it run faster?

The most obvious way I can see is to forgo the buffer. Just do:

template<class T> std::vector<T> getFromBin(std::ifstream& ifs, int num) {
    std::vector<T> output;
    output.resize(num);

    ifs.read(reinterpret_cast<char*>(output.data()), sizeof(T) * num);

    return output;
}

This is still the wrong way to read binary data for the reasons I mentioned above… but it is the exact same behaviour of your current code, without the unnecessary buffer.

(As an aside, even if the buffer were necessary, manually writing a loop and doing a bunch of push_back()s would be silly in this situation. Just do output.assign(buffer, buffer * num);.)

Incidentally:

std::ifstream ifs(file_name, std::ios::binary | std::ios::ate);
std::streampos total_bytes(ifs.tellg());
ifs.seekg(0, std::ios::beg);

This is not the right way to determine the size of a file. I’ve actually written articles about this, it’s such a common error. Simply put:

1. The number returned by tellg() is absolutely useless for any purpose other than to seekg() to the same location. In particular, if you tellg(), then write some data, then tellg() again, the difference between those two results is not necessarily the same as the number of bytes written.
2. The “end” of the file is not necessarily what you think it is. On some platforms, particularly those with record-based file systems, there can be a chunk of extra data at the “end” of the file. So, for example, even if tellg() could actually tell you the correct number of bytes, if you seekg() to the end of the file, you would still get the wrong number for the size.

There is no portable way to determine the file size other than by trying to actually read to the end, and counting the bytes you read. In other words, this is the only portable way to get the file size using IOstreams:

auto pos = ifs.tellg();
if (not ifs.ignore(std::numeric_limits<std::streamsize>::max()))
    throw "read failure";

auto total_bytes = ifs.gcount();
if (not ifs.seekg(pos))
    throw "seek failure";

// total_bytes now has the actual, correct number of bytes you can read
// between the current position and the end of the file.

As you can imagine, for a huge file, manually reading through the whole thing just to count bytes is a colossal waste of time. That’s why the pattern of checking the whole file’s size is not a good fit for IOstreams. If you don’t need to know the whole file’s size up front—and your use case, where you are reading chunk-by-chunk doesn’t seem to require knowing the whole file’s size—then don’t bother with it.

As a side note, if you want to know a file’s size portably, you can use std::filesystem::file_size(). But don’t do this:

auto total_bytes = std::filesystem::file_size(path);
auto ifs = std::ifstream{path, std::ios_base::binary};
// now assume the file size is total_bytes

There is a race condition between reading the file size and opening it. In short: you may read the file size, then the OS may pause your program to give time to another program, that program may delete the file and replace it with a totally new one, then the OS switches back to your program and you open the file stream.

The bottom line is that a stream interface is not really designed for the use pattern of getting the total size, and then reading it. A stream may not even know the total size… for example, if it is a network stream, and it is giving you data as it downloads without knowing the total download size. Yeah, sure, it should usually work for a file, on a local filesystem (but not always!), but the stream interface is for much more than just that.

The biggest issue I see with your current interface is that it doesn’t seem to have any idea that errors might occur, nor does it have a sensible way of handling them.

For example, let’s consider this simplified form:

template<class T> std::vector<T> getFromBin(std::ifstream& ifs, int num) {
    std::vector<T> output;
    output.resize(num);

    ifs.read(reinterpret_cast<char*>(output.data()), sizeof(T) * num);

    return output;
}

What happens if the read fails? You will happily return a vector half-full of data, and half-full of random garbage. The user has to write:

auto b = getFromBin<unsigned int>(ifs, 100000);

// is b good? don't know, have to check here:
if (not ifs)
    throw "problem";

It would make much more sense to do the checking in the function:

template<class T> std::vector<T> getFromBin(std::ifstream& ifs, int num) {
    std::vector<T> output;
    output.resize(num);

    if (not ifs.read(reinterpret_cast<char*>(output.data()), sizeof(T) * num))
        throw ...;

    return output;
}

That way you always know that when the function successfully returns, the data is good.

As for those Valgrind errors, as far as I can see, they are not related to any problems with memory allocation. As it says: All heap blocks were freed -- no leaks are possible. There is no way I can even begin to guess what’s going on without seeing the rest of your program, knowing the compiler and standard library you are using, and knowing the way you called Valgrind.