1
\$\begingroup\$

I'm doing some code review and I saw something like the following:

std::ifstream dataFile(filename);
if (dataFile.is_open())
{
    std::deque<std::string> lines;
    std::string line, line_batch;
    while (std::getline(dataFile, line))
    {
        if (!line.empty())
            lines.push_back(line);
    }
    dataFile.close();

    for (size_t i = 0; i < lines.size();)
    {
        if (i < MAX_BATCH_SIZE)
        {
            line_batch += "{" + lines.at(i) + "},";
            i++;
        }

I'm thinking that having many small strings (1000 lines equating to around 3000 strings) that will be concatenated together, that it would be beneficial to calculate the final string size ahead of time, reserve it and then do a concatenation using string::append as this would reduce fragmentation, reallocation and increase speed (the last one I don't think would be that big of a deal).

A read in line would, at a guess, be between 10-1000 characters. Max size of line_batch should be under 20MB. The application can get pretty big during execution. Big enough to force us to move from a 32 to a 64 bit memory scheme. The amount of times that this will happen depends on the user's environment, but if I were to guess, maybe between 0 and 100 times in a session. Could be more.

I'm a little on the fence on this. Am I being too nit picky? Is the added complexity worth it? If it matters, we're compiling with VS2019, though I don't think we're using it as a fully compliant c++17 compiler.

\$\endgroup\$
3
  • \$\begingroup\$ Do you need advice on how to benchmark it? \$\endgroup\$ Commented Feb 20, 2020 at 0:17
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Commented Feb 20, 2020 at 8:08
  • \$\begingroup\$ The code presented is a small fragment, there is not enough code here to do a review. \$\endgroup\$
    – pacmaninbw
    Commented Feb 20, 2020 at 12:38

1 Answer 1

1
\$\begingroup\$

This question is a bit too general. And I believe you are nitpicking but not for the reason you mentioned.

In the code it first reads all 1000+ lines and stores them into a deque. Each line is a potential allocation and deque also allocates linearly in the number of objects (one allocation for each 4k bytes and each strings takes 32 bytes... surely these numbers depend on platform and implementation and it works differently for large objects). Wouldn't that be a much much bigger source of memory fragmentation than just resizing 10 times one poor big string? IIRC string's capacity is in increased via the same policy as vectors, i.e., exponentially - thus it is just a few resizes.

I find the issue of storing the string as separate entities to be a much bigger problem than the big string. To deal with it, one needs to read each line and process it immediately (put it into the batchline and hopefully getline will reuse reserved data). At least I hope its enough. In this case one cannot figure out the final size of the string batch without reading the file twice... and nobody wants that. So you can either increase its size by appending or reserve at the start its maximal size and add elements till it hits the capacity and then forward it. Ofc, there is a small nuance of dealing with extra large strings bigger than the reserved size - but it is never too hard to fix. Just make sure you deal with it.

Disclaimer: I don't fully understand what brings more problems to memory fragmentation - it might be that deleting those strings at the end fixes most problems but if it so then why worry about the batch_string?

\$\endgroup\$
2
  • \$\begingroup\$ Hmmmm. You're right. The many small strings being read in is a bigger problem for fragmentation. I was actually considering more the use of the operator+, where it would generate an unnecessary string object. Reading in the file as one string and then breaking it up into lines (either with a vector of ranges or finding when needed) would be a better approach. \$\endgroup\$
    – Adrian
    Commented Feb 20, 2020 at 20:45
  • \$\begingroup\$ @Adrian implemented allocation strategies and what affects fragmentation more- is a tricky question. In my expirience reusing the same variable for quick temporary allocations is totally fin, almost as good as resereving reusing same allocation. It is surely platform dependent, tho. In this case compiler should be capable of optimizing away unnecessary copying and allocations with the + operator - but does it actually do it? It's a mystery. Generally, if you want to be safe from fragmentation issues consider using C++17 polymorphic allocator in your codebase. It should be the true fix. \$\endgroup\$
    – ALX23z
    Commented Feb 21, 2020 at 12:35

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