-1

I was asked to refactor some C++ code recently for the purposes of increasing unit testing coverage. The problem was that the code was tightly coupled on one compilation unit, so we had the equivalent of this:

beatles.h

class beatles { ... }

beatles.cpp

class john { ... }
class paul { ... }
class ringo { ... }
class george { ... }

...

// Implementation of beatles, john, paul, ringo and george classes ...

Because the 'internal' classes were only visible within the implementation file, they couldn't be unit tested very easily. Not only that, but the implementation file was huge and tricky to follow. I refactored the header and implementation to give something more like the following:

john.h

class john { ... }

paul.h

class paul { ... }

ringo.h

class ringo { ... }

george.h

class george { ... }

beatles.h

#include <john.h>
#include <paul.h>
#include <ringo.h>
#include <george.h>

class beatles { ... }

For simplicity, I've omitted that each one has its own separate implementation file. Naturally this is a drastic simplification but hopefully it illustrates the point.

Now we can add unit tests for the classes that previously couldn't be tested, which is great. However, for some of the operations at runtime we now see a significant performance hit, which we've concluded is due to the compiler not being able to optimise as much as it previously was, as there are absolutely no functional differences in this change.

Now, I'm aware of the benefits of loose coupling and strong cohesion. However, there comes a point where other factors such as code maintainability and testing etc. also start to come into this. I have to admit that I'm also a little uncomfortable that simply structuring our code one way or another can have such a profound, and potentially unpredictable affect on aspects such as performance.

My question is this: Is it plausible to think that the optimiser is at the root of this issue and, if so, is there a way that we can structure the code to allow the public interface for testing purposes while not throttling the optimiser?

8
  • 2
    "Are we right to think that the optimiser is at the root of the issue" How should we know? You've given us 5 lines of pseudo-code with zero actual functionality. May 13 at 10:42
  • Aside: "absolutely no functional differences in this change." You've just moved declarations from being visible only in beatles.cpp to everywhere that include beatles.h, and definitions out of beatles.cpp
    – Caleth
    May 13 at 10:43
  • 1
    It's almost certainly to do with inlining. But you've got to do the digging when there's a performance issue. Run a number of iterations under a sampling profiler and see where the hotspots are, before and after. Go look at the disassembly and see whether the inlining has changed.
    – pjc50
    May 13 at 15:49
  • 1
    Other possibilities: string coalescing in the linker? Alignment changing cache behavior, in either the i-cache or the d-cache? It sounds like there's a single very tight loop involved, so you need to identify that and put it under a microscope.
    – pjc50
    May 13 at 15:51
  • 1
    Did the refactoring change linkage or inline status of the functions/methods? For example, methods defined within a class definition like class john { void foo() {} } are implicitly inline, whereas separate definitions like class john { void foo(); } void john::foo() {} are not. Linkage refers to compilation-unit level visibility. Functions with internal linkage (like static functions or functions in anonymous namespaces) and inline functions can be optimized more heavily. Optimizations across compilation units are only possible with link-time optimization (LTO).
    – amon
    May 14 at 6:19

2 Answers 2

3

You didn't say what percentage in performance change. And whether it is for a tight benchmark or as part of an application.

You might see a function getting ten times slower - from 1 nanosecond to 10 - which turns out to be completely irrelevant. Maybe another function changes from 1,000 to 1,009 nanoseconds for the same reason, and that's even more irrelevant.

The optimiser might see that calling function f() has no side effects. If you call it in a loop a billion times it still has no side effect, so that loop has zero runtime. But if the function is hidden in another compilation unit, the compiler doesn't know, and makes a billion calls.

Performance of benchmarks is irrelevant. What counts is performance of actual production code.

You might also check if you haven't made other changes unintentionally. For example, you might have turned optimisation off while refactoring for easier debugging, and not turned it on again.

4
  • Useful comments. For some benchmarks there was no change, but one or two increased 100x. The benchmarks are a mixture of realistic use cases and extreme scenarios but the problematic ones. While I doubt it is completely irrelevant, I'm unlikely to have a good case for putting code forward for production is the change introduces a consistent degradation in any performance test. May 13 at 11:27
  • 1
    Can you check actual CPU usage while the packages are running? Extreme case would be 1 second with CPU at 100%, vs. 100 seconds with CPU at 1%. Can happen with multithreading. If that's not the problem, it's time to get a profiler out...
    – gnasher729
    May 13 at 15:32
  • 1
    If it's a performance problem, it's always time to get the profiler out.
    – pjc50
    May 13 at 15:52
  • The profiler will be used, that's for certain, though I have a slight concern that a profiler build will itself affect the behavior of the code. May 16 at 0:22
2

It's certainly plausible that moving definitions to a new translation unit has meant the optimiser couldn't do as well.

You can #include whatever files you like, so you can re-create the old structure as seen by the compiler while keeping the new files.

beatles.h

class beatles { ... }

beatles.cpp

#include "john.cpp" // transitively #includes "john.h"
#include "paul.cpp"
#include "george.cpp"
#include "ringo.cpp"

...

// Implementation of beatles

test_john.cpp

#include "john.h"

You will have to be careful that you don't try to link beatles.o and john.o in the same executable.

This structure will surprise some people, so you probably want a big warning at the top of beatles.cpp along the lines of

IMPORTANT! We found an unacceptably long (yyy seconds wall clock time) performance degradation splitting these translation units (under compiler(s) foo x.y.z bar h.j.k)

Do not rearrange unless you can show they all now have better optimisers

1
  • 1
    Definitely agree with the warning comment. Had one like that existed before I made this change, my approach would have been very different. May 13 at 11:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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