Optimizing loop and avoiding code duplication in a template specialization

Question

Assume we have a function

template< typename A, typename B, typename C >
void function(vector<A>& keyContainer, int a, int b, int c, boost::function<bool(B&)> selector, C* objPointer = NULL)
{
    BOOST_FOREACH(const A& key, keyContainer)
    {
        B* pVal = someGetObjFunction(objPointer, key);
        if(pVal)
        {
            if(selector && selector(*pVal))
            {
                pVal->someOtherFunction(1,2,3);
            }
            //some more code
        }
    }
}

This looks bad because it will always enter the

if(selector && selector(*pVal))

even when it is NULL, an obvious approach to fix this would be :

template< typename A, typename B, typename C >
void function(vector<A>& keyContainer, int a, int b, int c, boost::function<bool(B&)> selector, C* objPointer = NULL)
{
    if(selector)
    {
        BOOST_FOREACH(const A& key, keyContainer)
        {
            B* pVal = someGetObjFunction(objPointer, key);
            if(pVal)
            {
                if(selector(*pVal))
                {
                    pVal->someOtherFunction(1,2,3);
                }
                //some more code
            }
        }
    }
    else
    {
        BOOST_FOREACH(const A& key, keyContainer)
        {
            B* pVal = someGetObjFunction(objPointer, key);
            if(pVal)
            {
                pVal->someOtherFunction(1,2,3);
                //some more code
            }
        }
    }
}

But this resulted in a lot of code duplication, another approach would be making a specialization for the case when the function is NULL but wouldnt that be almost identical as the example above? Is there another way of doing that without duplicating all the code ?

Have you measured the original solution and discovered that checking `selector` for `NULL` is the bottleneck, and that your alternative solution results in a noticeable improvement? If you care about performance, you would measure it. If you haven't measured, that shows you don't really care. "Premature optimization is the root of all evil."~~Donald Knuth. — Igor Tandetnik, Aug 16 '14 at 12:56
Some compilers can conditions inside loops pretty well. If it csn prove the `bool` value of `selector` is unchanging. — Yakk - Adam Nevraumont, Aug 16 '14 at 13:12
You may test `selector` once, and when null, set it to a default function (or lambda `[](B&){return true;}`). — Jarod42, Aug 16 '14 at 13:20
He will only check the selector for null (2 machine instructions) in the loop, because the `&&` is only evaluated if selector is non null. As you know at compile time that it's zero, the optimizer should eilminate it. *Never do painfully what the compiler can do for you much better.* — Christophe, Aug 16 '14 at 13:24

score 1 · Accepted Answer · answered Aug 16 '14 at 13:21

Your problem description is a bit confusing. You are not checking for NULL, because selector is not a pointer. Instead, you are checking to see if the boost::function object is empty() see: http://www.boost.org/doc/libs/1_55_0/doc/html/boost/function.html#idp54857000-bb.

Also, your two blocks of code are not equivalent. Your seem to indicate that you want to execute the inner loop if either (selector is provided and true) or (selector is not provided).

So, your first code block should be:

template< typename A, typename B, typename C >
void function(vector<A>& keyContainer, int a, int b, int c, boost::function<bool(B&)> selector, C* objPointer = NULL)
{
    BOOST_FOREACH(const A& key, keyContainer)
    {
        B* pVal = someGetObjFunction(objPointer, key);
        if(pVal)
        {
            if(!selector || (selector && selector(*pVal)))
            {
                pVal->someOtherFunction(1,2,3);
            }
            //some more code
        }
    }
}

This is logically equivalent to your second block of code.

As Igor Tendetnik mentioned, you need to actually measure the code to see where the bottleneck is. It's likely not the checking for selector being empty.

If checking to see if the selector is empty really is your bottleneck, which is unlikely because the compiler with optimizations turned on is going to make those comparisons very fast inlined function calls, you could cache the result of the empty test.

template< typename A, typename B, typename C >
void function(vector<A>& keyContainer, int a, int b, int c, boost::function<bool(B&)> selector, C* objPointer = NULL)
{
    bool check_selector = !selector.empty();
    BOOST_FOREACH(const A& key, keyContainer)
    {
        B* pVal = someGetObjFunction(objPointer, key);
        if(pVal)
        {
            if(!check_selector || (check_selector && selector(*pVal)))
            {
                pVal->someOtherFunction(1,2,3);
            }
            //some more code
        }
    }
}

Optimizing loop and avoiding code duplication in a template specialization

1 Answers1