6
\$\begingroup\$

I am trying to read entries of my CSV file into an entries vector whose data type is object OrderBookEntry, which is an object that stores the tokenized variables from the CSV line which itself is converted through the function tokenise. My problem is that my CSV file has around a million lines and it takes over 30 seconds to load; how can I reduce this?

This is my code that actually reads lines from the file and converts them into entries.

    std::vector<OrderBookEntry> CSVReader::readCSV(std::string csvFilename)
    {
        std::vector<OrderBookEntry> entries;
    
        std::ifstream csvFile{csvFilename};
        
        std::string line;
        if (csvFile.is_open())
        {
            while(std::getline(csvFile, line))
            {
                try {
                    OrderBookEntry obe = stringsToOBE(tokenise(line, ','));
                    entries.push_back(obe);
                    //std::cout << "pushing entry" << std::endl;
                }catch(const std::exception& e)
                {
                    //std::cout << "CSVReader::readCSV bad data"  << std::endl;
                }
            }// end of while
        }    
    
        std::cout << "CSVReader::readCSV read " << entries.size() << " entries"  << 
        std::endl;
        return entries; 
    } 

This is the part of my code that tokenises the read line:

    std::vector<std::string> CSVReader::tokenise(std::string csvLine, char separator)
    {
       std::vector<std::string> tokens;
       signed int start, end;
       std::string token;
        start = csvLine.find_first_not_of(separator, 0);
        do{
            end = csvLine.find_first_of(separator, start);
            if (start == csvLine.length() || start == end) break;
            if (end >= 0) token = csvLine.substr(start, end - start);
            else token = csvLine.substr(start, csvLine.length() - start);
            tokens.push_back(token);
        start = end + 1;
        }while(end > 0);
    
       return tokens; 
    } 

This is the part of my code that converts the tokens into a suitable object to work with:


    OrderBookEntry CSVReader::stringsToOBE(std::vector<std::string> tokens)
    {
        double price, amount;
    
        if (tokens.size() != 5) // bad
        {
            //std::cout << "Bad line " << std::endl;
            throw std::exception{};
        }
        // we have 5 tokens
        try {
             price = std::stod(tokens[3]);
             amount = std::stod(tokens[4]);
        }catch(const std::exception& e){
            std::cout << "Bad float! " << tokens[3]<< std::endl;
            std::cout << "Bad float! " << tokens[4]<< std::endl; 
            throw;        
        }
    
        OrderBookEntry obe{price, 
                            amount, 
                            tokens[0],
                            tokens[1], 
                            OrderBookEntry::stringToOrderBookType(tokens[2])};
    
        return obe; 
    } 

Do advise on how I can make the code faster.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ What is OrderBookEntry? \$\endgroup\$
    – convert
    Dec 27, 2022 at 13:17
  • \$\begingroup\$ Welcome to the Code Review Community. We could provide even more help with optimization if we knew more about the code. Please don't change this question but in the future, the more code you provide the better we can help you. Please read How do I ask a good question? and A guide to Code Review for Stack Overflow users \$\endgroup\$
    – pacmaninbw
    Dec 27, 2022 at 16:30
  • \$\begingroup\$ In addition to the answers below, when transitioning to string_view use methods from <charconv> like from_chars they convert numbers much faster than stod. Assuming, your compiler supports them ofc. Also, why amount is a double? Shouldn't it be integer? \$\endgroup\$
    – ALX23z
    Dec 28, 2022 at 10:57

2 Answers 2

4
\$\begingroup\$

Avoid throwing exceptions if they are not exceptional

You throw exceptions on parse errors, but in C++, exceptions are free if you don't throw them (i.e., you don't pay for the try-catch), but they can be very slow when you do throw. So you should only use that for exceptional situations, as the name implies. If your input file has lots of empty lines or lines with comments instead of data, then you throw exceptions a lot, slowing down your code significantly. Consider using a different way to signal that a line could not be parsed, for example by returning a std::optional<OrderBookEntry> from stringsToOBE().

Avoid memory allocations

std::vector and std::string perform memory allocations under the hood. As slepic mentioned, pass std::string by const reference where possible. But you can also consider using std::string_view. In particular, tokenise() could take csvLine as a std::string_view and return a std::vector<std::string_view>. This way it doesn't pay for string allocations.

If you know each line should have exactly 5 tokens, then you could also consider returning a std::array<std::string_view, 5>.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Parse errors may be legitimately considered exceptional, depending on whether you expect input to be valid or not. \$\endgroup\$ Dec 27, 2022 at 14:30
5
\$\begingroup\$

Don't pass strings and vectors by value. Pass them by const reference. When you pass them by value, they need to be copied which is a waste of time and resources.

std::vector<std::string> CSVReader::tokenise(const std::string & csvLine, char separator);

and

OrderBookEntry CSVReader::stringsToOBE(const std::vector<std::string> & tokens);

and lastly here, but here it is not going to be so big problem in terms of performance because it is only called once.

std::vector<OrderBookEntry> CSVReader::readCSV(const std::string & csvFilename);

Also make sure that OrderBookEntry constructor does not suffer the same problem.

Also when you are using vector you effectively copy the strings again if you push_back them to the vector, you better emplace_back them. Or even better, use std::string_view.

In either case, the same goes for vector of order book entries. You should emplace_back the entries to avoid copying the entries.

entries.emplace_back(stringsToOBE(tokenise(line, ',')));
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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