Minimizing code duplication without using macros or sacrificing speed

Question

Here's a snippet of code:

An inlined function:

inline void rayStep(const glm::vec3 &ray, float &rayLength, const glm::vec3 &distanceFactor, glm::ivec3 &currentVoxelCoordinates, const glm::ivec3 &raySign, const glm::ivec3 &rayPositive, glm::vec3 &positionInVoxel, const int smallestIndex) {
    rayLength += distanceFactor[smallestIndex];
    currentVoxelCoordinates[smallestIndex] += raySign[smallestIndex];
    positionInVoxel += ray * glm::vec3(distanceFactor[smallestIndex]);
    positionInVoxel[smallestIndex] = 1 - rayPositive[smallestIndex];
}

It's usage:

glm::ivec3 distanceFactor = (glm::vec3(rayPositive) - positionInVoxel) / ray;

    if (distanceFactor.x < distanceFactor.y)
    {
        if (distanceFactor.x < distanceFactor.z)
        {
            rayStep(ray, rayLength, distanceFactor, currentVoxelCoordinates, raySign, rayPositive, positionInVoxel, 0);
        }
        else
        {
            rayStep(ray, rayLength, distanceFactor, currentVoxelCoordinates, raySign, rayPositive, positionInVoxel, 2);
        }
    }
    else
    {
        if (distanceFactor.y < distanceFactor.z)
        {
            rayStep(ray, rayLength, distanceFactor, currentVoxelCoordinates, raySign, rayPositive, positionInVoxel, 1);
        }
        else
        {
            rayStep(ray, rayLength, distanceFactor, currentVoxelCoordinates, raySign, rayPositive, positionInVoxel, 2);
        }
    }

I really dislike the way the usage of the function looks like. One way I could fix it is to calculate the index of the smallest component and then just use the body of the function directly in the code:

int smallestIndex = (distanceFactor.x < distanceFactor.y) ? (distanceFactor.x < distanceFactor.z ? 0 : 2) : (distanceFactor.y < distanceFactor.z ? 1 : 2);
rayLength += distanceFactor[smallestIndex];
currentVoxelCoordinates[smallestIndex] += raySign[smallestIndex];
positionInVoxel += ray * glm::vec3(distanceFactor[smallestIndex]);
positionInVoxel[smallestIndex] = 1 - rayPositive[smallestIndex];

This looks much cleaner to me.

So why haven't I done that, if it bothers me so much?

The benefit of the above code is that value of the smallestComponentIndex of the function is know at compile time - the value is given as a constant and the function is inlined. This enables the compiler to do some optimizations which it wouldn't be able to do if the value were unknown during the compile time - which is what happens in the example with double ternary operator.

The performance hit in my example is not small, the code goes from 30ms to about 45ms of execution time - that's 50% increase.

This seems negligible, but this is a part of a simple ray tracer - if I want to scale it to do more complex calculations, I need this part to be as fast as possible, since it's done once per ray intersection. This was run on low resolution with a single ray per pixel, with no light sources taken into account. A simple ray cast, really, hence the runtime of 30ish ms.

Is there any way I can have both the code expression and speed? Some nicer way to express what I want to do, while making sure that value of the smallestComponentIndex is known at compile time?

Answer 1

If you use C++11, you can wrap the duplicated code in a local lambda function. Since a lambda is guaranteed to be inlined, performance won't be affected.

auto step = [&](int index)
{
    rayStep(ray, rayLength, distanceFactor, currentVoxelCoordinates, raySign, rayPositive, positionInVoxel, index);
};
if (d.x  < d.y && d.x  < d.z) {
    step(0);
}
else if (d.y <= d.x && d.y < d.z) {
    step(1);
}
else {
    step(2);
}

Answer 2

Refactor for maintainability, and only try for more efficiency if needed.

So, extract the index-calculation:

static constexpr int smallestIndex(glm::ivec3 v) noexcept {
    return v.x < v.y && v.x < v.z ? 0 : v.y < v.z ? 1 : 2;
}

A more literal transcription might be optimized differently though:

static constexpr int smallestIndex(glm::ivec3 v) noexcept {
    return v.x < v.y
        ? (v.x < v.z ? 0 : 2)
        : (v.y < v.z ? 1 : 2);
}

And use that to simplify your code:

glm::ivec3 distanceFactor = (glm::vec3(rayPositive) - positionInVoxel) / ray;
rayStep(ray, rayLength, distanceFactor, currentVoxelCoordinates, raySign, rayPositive, positionInVoxel, smallestIndex(distanceFactor));

Most compilers have annotations for forcing inlining, like __attribute__((always_inline)) for gcc. Try that before sacrificing readability, if actually necessary.