4
\$\begingroup\$

I'm parsing STL files to load the coordinates. My code is:

void Importer::scanFileSTL_ASCII(
    QVector<QVector3D> &vertices, // Coordinates to be filled 
    const QUrl &url)
{
    QFile file;
    file.setFileName(url.toLocalFile());
    if (!file.open(QIODevice::OpenModeFlag::ReadOnly | QIODevice::OpenModeFlag::Text))
        return;

    while (!file.atEnd()) {
        QByteArray line_BA = file.readLine();
        QString line = QString::fromLatin1(line_BA);
        // Split line by spaces
        QStringList words = line.split(QRegExp("\\s+"), QString::SkipEmptyParts);
        if (words.isEmpty())
            continue; // Skip this line and jump to next iteration/line
        if (words[0] == "vertex") {
            assert (words.length() == 4 && "STL file format is wrong: vertex-line must have 4 strings");
            float x = words[1].toFloat();
            float y = words[2].toFloat();
            float z = words[3].toFloat();
            vertices.append(QVector3D(x, y, z));
        }
    }

    return;
}

The code is tested, it works fine. But it is by far slower than the STL loader of asset import library. More accurate comparisons in release mode:

Import of a 13MB ASCII STL file:

  • Assimp library: 03.58 seconds
  • Our code: 04.70 seconds

Import of 287MB ASCII STL file:

  • Assimp library: 05.72 seconds
  • Our code: 44.19 seconds

I wonder if something is noticeably wrong with my code. Thanks.

\$\endgroup\$

closed as off-topic by πάντα ῥεῖ, dfhwze, Mast, Grajdeanu Alex., AlexV Jul 24 at 17:52

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – πάντα ῥεῖ, dfhwze, Mast, Grajdeanu Alex., AlexV
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Can you please guide me how to improve my question, if it is unclear? \$\endgroup\$ – user4838962 Jul 20 at 7:58
  • 1
    \$\begingroup\$ As a side notice, if you don't want to use assimp, you still might want to use vcg, the self-contained library behind MeshLab. \$\endgroup\$ – Oleg Lobachev Jul 20 at 22:42
  • 1
    \$\begingroup\$ Can you share how you test this? Results of a profile are next-to-useless if we can't see how it was retrieved. For general question-improvement advice, see this FAQ. \$\endgroup\$ – Mast Jul 21 at 14:33
7
\$\begingroup\$

Well, it's not much of a mystery. Your code is simple and pretty. Their code is ugly because it performs exactly what it needs to do, and no more:

} else if (!strncmp(sz,"vertex",6) && ::IsSpaceOrNewLine(*(sz+6))) { // vertex 1.50000 1.50000 0.00000
    if (faceVertexCounter >= 3) {
        ASSIMP_LOG_ERROR("STL: a facet with more than 3 vertices has been found");
        ++sz;
    } else {
        if (sz[6] == '\0') {
            throw DeadlyImportError("STL: unexpected EOF while parsing facet");
        }
        sz += 7;
        SkipSpaces(&sz);
        positionBuffer.push_back(aiVector3D());
        aiVector3D* vn = &positionBuffer.back();
        sz = fast_atoreal_move<ai_real>(sz, (ai_real&)vn->x );
        SkipSpaces(&sz);
        sz = fast_atoreal_move<ai_real>(sz, (ai_real&)vn->y );
        SkipSpaces(&sz);
        sz = fast_atoreal_move<ai_real>(sz, (ai_real&)vn->z );
        faceVertexCounter++;
    }

A few differences in the amount of work it does:

  • There's no regex that needs to be compiled.
  • It's not splitting tokens into a list. So, no list needs to be allocated and filled. More importantly, no strings need to be allocated, copied, and destroyed.
  • The aiVector3D is allocated first, then each of the three numbers is parsed and written directly into the memory location of the coordinate, with no temporary variable assignments.

I'll also point out that your use of assert is improper. You should only assert conditions that you know must be true due to logic. You should not assert conditions that you hope to be true about the input. If you compile with assertions disabled, then the validation code goes away! That would introduce a bug, and possibly a security vulnerability.

\$\endgroup\$
5
\$\begingroup\$

Note that we don't have a fully-compilable program, or the example data. But you do! So you can actually measure what's taking up the time using a profiler.


However, I would guess that the following lines are the problem:

    QString line = QString::fromLatin1(line_BA);
    // Split line by spaces
    QStringList words = line.split(QRegExp("\\s+"), QString::SkipEmptyParts);

I guess that fromLatin1() effectively copies the QByteArray (i.e. memory allocation, then iterate over everything and copy it).

Then the regex will split every word, which involves something like iterating over the string to match the regex, memory allocation for each separate word, then copying each word.

None of this processing / memory allocation / copying is actually necessary to extract the data required.

Using the C++ standard library instead of Qt things, we could do something like:

#include <algorithm>
#include <cctype>
#include <cerrno>
#include <fstream>
#include <iostream>
#include <string>

bool try_parse_float(char const*& start, char const* end, float& value)
{
    errno = 0;
    auto cend = const_cast<char*>(end);
    float v = std::strtof(start, &cend);

    if (end == start)
        return false; // no conversion.

    if (errno == ERANGE)
        return false; // float out of range.

    // success!
    value = v;
    start = cend;

    return true;
}

bool parse(std::string const& filename)
{
    std::ifstream file(filename);

    if (!file)
        return false;

    std::string line;
    while (std::getline(file, line))
    {
        auto i = line.data();
        auto end = line.data() + line.size();

        // parse whitespace
        while (i != end && std::isspace(static_cast<unsigned char>(*i)))
            ++i;

        // empty line
        if (i == end)
            continue;

        // check for "vertex" prefix
        char const prefix[] = "vertex";
        auto prefix_size = sizeof(prefix) - 1;
        auto starts_with = std::strncmp(i, prefix, std::min<std::size_t>(prefix_size, end - i));

        if (starts_with != 0)
            return false; // "vertex" is missing or malformed.

        i += prefix_size;

        // convert floats (discards leading whitespace internally)
        float x;
        if (!try_parse_float(i, end, x))
            return false;

        float y;
        if (!try_parse_float(i, end, y))
            return false;

        float z;
        if (!try_parse_float(i, end, z))
            return false;

        // parse any trailing whitespace
        while (i != end && std::isspace(static_cast<unsigned char>(*i)))
            ++i;

        if (i != end)
            return false; // there's something else on this line!

        std::cout << "vertex: " << x << " " << y << " " << z << "\n"; // (... or copy vertex to vector)
    }

    return true;
}

int main()
{
    std::cout << (parse("test.stl") ? "success" : "failure") << "\n";
}

Which is rather more fiddly, but does no extra copying. The messiness can be hidden neatly into separate functions if necessary.


As a side note, you might want to return a bool indicating success / failure from that function. At the moment there would be no obvious difference between failing to read a file, or a valid file that just contains no data.

\$\endgroup\$

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