Debugging a performance issue, do I commit the timing code?

Consider adding metrics to your code.

Record the timings of various critical paths, how many threads are in use, api calls are made etc. Output these to some performance monitoring tool. eg Windows Perfmon

This can be done without adding much overhead and will provide valuable data for investigation and diagnosis of problems. eg. Does it run slow on my computer/when the virus checker runs/as you open more tabs/with certain config values.

I wouldn't go as far as measuring the timing of every individual function. That's what debug tools are for. But some higher level stuff should serve as an alert which prompts you to investigate more closely

Do you expect to use this timing code ever again? I have written code to find a bug, found the bug, and thrown the code away because it would never be used again. Timing code, you might check in with other timing code and documentation how to use it.

This kind of code you want to run sometimes. Which means if you run it, you change something in your code (like turning the timing code on). I'd probably have a header file "timingcode.h", a big #define that turns off timing completely, and individual #defines for each item where you have individual timing code. So if someone has the desire to get timing information for feature XYZ, they check out the latest version, go into "timingcode.h", #define the global switch enabling timing, and #define for example timing_XYZ, then run the code.

You probably want to build a version that is as close as possible to released code. Unless your problem is debugging code that takes so long it interferes with development work, and you want to get timing information for that.

Note: If you have a clueless manager who judges your performance and pays bonuses according to commits, then you obviously commit the code, even if you know 100% that the commit is pointless and stupid and gives no benefits whatsoever. If management rewards you for doing stupid things then you do stupid things and nobody will blame you for it.

In the general case, I will typically wrap test code like this in a #ifdef so that it's there if you need it, but it's not actually part of the build unless you go out of your way to turn it on.

That's only for test code of meaningful complexity, though. If this is something as simple as wrapping a function call with a timer start/stop, then I would argue that it's so trivial and easy to recreate that it's not worth the overhead and hassle of keeping (I would rank that like adding a printf call to see the value of a variable). If the test code requires extensive setup or has to be measured in a very specific way, then it might be significant enough to be worth keeping.

For this type of performance measurement in particular, I recommend not writing this sort of code at all and instead using a code profiling tool (valgrind, Intel VTune, etc) as part of your standard test routine. These tools will automatically insert timing and tracking code to every function call and will generate a much more comprehensive performance analysis. It's also all completely automatic, so you don't have to add anything to your code at all.

Now the question is, how do I deal with the added code that measures the execution time of the function? Do I commit it?

Yes do commit stuff like this as temporary commits, where temporary means something you (or your colleagues) want to have present while working on the branch but eventually want to remove before merging the result in the end.

The key concept to temporary commits is to create them with commit messages that clearly differentiate them from normal commits so they are trivial to remove. My approach is to add a "====" pre- and post-fix, e.g.

git add -p src/f1.cpp
git commit -m "==== f1 timing debug ===="

With such commit messages they stand out like sore thumbs and it is trivial (even automatable) to remove before making a release.

If I don't commit it, other developers who review my code have to add back the performance measuring code to their local copy to test my revision.

With temporary commits you only need to agree on a naming convention. Then you can freely share your debug code in a "safe" way where your colleagues intuitively understands that this timing debug is temporary and still benefit from the functionality.

And if you want to keep some of these temporary commits for usage after finishing the current branch you are working on there is no problem keeping them on a different branch (e.g. debug) which you can rebase to wherever you want, they are after all just normal commits you just treat them differently.

if you have existing unit tests for your codebase, consider adding this as a unit test for the function f1. I've typically seen the unit test files categorised as either func (functional) or perf (performance), with functional testing being focussed on the function always returning valid information, even in corner-cases, and performance tests being focussed on timings (in whatever metric is important to you, ie: thoroughput, worst case, etc..). you can then link in to a more mature performance tool such as google benchmark, which does a bit of data shaping to avoid issues with caching, etc..

You could just leave it at that, the perf unit tests would just spit out a bunch of timing information to stdout, or you could set assertions to prove that your timing isn't regressing, and have them run in CI.

Here’s one way to commit code without releasing it, but be prepared to explain:

1. Commit the timing code. You can do this on a branch based on your current branch tip, or if you’re comfortable with Git do the commit in a detached head (or on your current branch and then git reset --hard HEAD~)
2. Merge the new branch (or commit hash) into your branch with -s ours (the ours strategy). This creates a multi-parent commit (a merge) but whose tree only reflects the original branch. The second parent still shows the added code.

Notes:

• This is similar to an evil merge where the result contains code from neither side, except that in this case we drop code that appeared in one side
• This is entirely different from -X ours, which is a strategy option that instructs the default merge strategy to resolve conflicts using our side.
• It’s worth explaining what you did and why in the merge commit message.