A mini-break in C++ Optimization (Pt 1)

I have had one of those evenings, in reviewing some code for a friend I came across a repeated pattern where he would iterate over collections, strings and other linear items in a loop and mutate them.  We're of course talking C++ here, and he had this whole myriad of loops all over sanitizing, processing, event summing values.

I asked whether he had access to the STL he immediately got a bit uppity, he and I have never seen eye to eye about using the STL.

I advocate it be used, even just standing something up which you go back and make more performant or more memory efficient later, just stand up your project, don't get bogged down in details of the best string, or the best vector.

He however stands by his having "long tested examples I use all the time, just ignore them"...

No.

Simply put his examples, used in a smattering of programs, peer reviewed very infrequently by a miniscule handful of people is not good enough, use the STL, it is seen by literally millions of engineers, it is very portable and well adopted, use it to your advantage.

We sparred a little about this, but then I simply asked whether he had any parallelism in his algorithms?

Nope, of course STL can just use them.... You can provide the execution policy.

Lets take one of his examples, a simple function to give him the time & date as a character array.

char* GetDateTimeStringHis()
{
time_t now = time(0);
char* buffer = new char[64];
memset(buffer, 0, 64);
ctime_s(buffer, 63, &now);
for (int i = 0; i < 64; ++i)
{
if (buffer[i] == '\n') buffer[i] = 0;
}
return buffer;
}

This returns a character array buffer raw pointer.... No, he interjected, it returns a string.  A raw pointer to memory is NOT a string folks, this is where my argument on that point begins and ends.  This code is very unsafe, I don't like it and I certainly insist it can easily leak memory, for whom is going to delete that buffer?

So I erase that buffer with an actual standard string:

        std::string GetDateTimeStringV1()
{
time_t now = time(0);
std::string buffer(64, 0);
ctime_s(buffer.data(), 63, &now);
for (int i = 0; i < 64; ++i)
{
if (buffer[i] == '\n') buffer[i] = 0;
}
return buffer;
}

he didn't like this, but accepted perhaps that was better, he argued about the string constructor taking a length and a character (zero) to initialize itself with, but went with it.  It's safer, simpler, easier to read code.

I'm still not happy, I don't like that loop:

        std::string GetDateTimeStringV1()
{
time_t now = time(0);
std::string buffer(64, 0);
ctime_s(buffer.data(), 63, &now);
std::transform(
buffer.begin(), buffer.end(), buffer.begin(),
[](const char character) -> char
{
if (character == '\n')
{
return 0;
}
else
{
return character;
}});
return buffer;
}

He hated this, and I have to be honest I don't like the lambda either, but this is correct, using the transform algorithm and the lambda could be expressed in a location it could be reused, or just as a free function somewhere, it doesn't have to be inline here making a mess of reading the code.

What however is it doing?  It changes any new line to a zero in the buffer string, why?  Well ctime_s puts a new line at the end, so if we reverse the iteration we may get a much better performance we can early out of the loop.  But we can even just do away with the whole transform, we know it's the last character so just set it to zero as an optimization and simplification.  My friend can get behind this now:

        std::string GetDateTimeStringV2()
{
time_t now = time(0);
std::string buffer(64, 0);
ctime_s(buffer.data(), 63, &now);
buffer[std::strlen(buffer.data())-1] = 0;
return buffer;
}

We can even further think about this code, what will strlen do?  It might count from 0 to N to find the first zero character as the value, could we therefore count backwards from the 64th position in the buffer?  Well, we know the format of the data and time we get is known:

        Wed Mar 29 23:36:01 2023

Twenty five characters with the new line, adding another for the null termination character, so we can always just set the twenty fourth to zero and reduce the buffer size at the same time to perfectly fit!

        std::string GetDateTimeStringV3()
{
time_t now = time(0);
std::string buffer(26, 0);
ctime_s(buffer.data(), 26, &now);
buffer[24] = 0;
return buffer;
}

I feel I'm getting somewhere, I just want to make sure we use the STL version of the time functions now and use brace initialization for the "now".

        std::string GetDateTimeStringV4()

{

std::time_t now{ std::time(0) };

std::string buffer(26, 0);

ctime_s(buffer.data(), 26, &now);

buffer[24] = 0;

return buffer;

}

This ended my concerns and improved the performance of the code, he was calling this function nearly every time he logged, nearly every time he sent a network message and for very many of his database transactions.  This was 10 minutes of just thinking about the problem.

Now... Back to <algorithm> and execution policy with him...

C++ Variadic Template Compiler Optimizations

In my prior post we briefly talked about how the compiler unrolls a variadic template and we needed to provide the end-point function to stop that iteration, lets take a closer look at how the compiler does this unrolling.

The impact this has on your debug build is critical, and I demonstrate that you can see the unrolling of numeric functions (such as summation) but then in release the compiler is smart enough to optimize that all away and use a constant.  Understanding when this is happening is a crucial lesson, as we then see the action of a repetitive string output.

If you found these two little videos of any use, please life & subscribe on YouTube to let me know!

And as always the comments below are available.

C++ : Copy Constructor versus Ignorance

I've spent a bit of time reviewing someone else's code recently and I've come to an impasse with them, so they have a lot of code which will take some standard container, and the code doesn't just initialise the local copy from the passed in reference... No it'll iterate over the list of elements adding them to the class version.

I have picked fault with this, it's not RAII, it looks ugly and if you're threading you can create your class instance and the member is empty or in a partially filled state before the loop within the constructor is finished... I highlight this in red below...

My solution?  Just initialise the member from the reference - see the green highlight in the code below.

My results from the timing below?

These times are "microseconds" so tiny... But with just constructing from the existing reference we always get a lower time, quicker code...

Running this test 30,000 times, trying it in different orders and with maps of upto 1000 elements I had a rough average increase of 60% speed by using the copy constructor of std::map rather than reallocating new memory for each pre-existing element.

I wanted therefore to understand the reasoning behind the original code, was there some reason to perform a clone (alloc and assign new instance) operation for each pair in the map being passed in?  Looking at the code there was no apparent reason, so I spoke to the developer, asking why they had performed the construction in this manner... Their reply...

"That's how you initialise a container"

You can load up a container in this manner, but you already have the contents of the map, you're just copying it... "Why don't you use the copy constructor?"... I asked...

"I didn't write one"

Hmm, "You don't, the compiler generates it for you, std::map has its own copy constructor".  Use the copy constructor folks, trust me.

#include <map>
#include <chrono>
#include <string>
#include <iostream>


using Mapping = std::map<int, int>;

class A
{
private:
Mapping m_TheMapping;

public:

A() = delete;
A(const A&) = delete;
void operator=(const A&) = delete;

A(const Mapping& p_Mapping)
:
m_TheMapping(p_Mapping)
{
}

A(const Mapping& p_Mapping, const bool& p_Other)
:
m_TheMapping()
{
for (auto i : p_Mapping)
{
m_TheMapping.emplace(i);
}
}

inline Mapping& Mapping()
{
return m_TheMapping;
}
};


int main()
{
Mapping l_m
{
{ 0, 0 },
{ 1, 1 },
{ 2, 2 },
{ 3, 3 }
};

auto l_time(std::chrono::high_resolution_clock::now());
// Copy Construction
A l_map(l_m);
auto l_timeB(std::chrono::high_resolution_clock::now());

auto l_Dur(l_timeB - l_time);
std::cout << std::chrono::duration_cast<std::chrono::microseconds>(l_Dur).count() << std::endl;


auto l_time2(std::chrono::high_resolution_clock::now());
// Clone Construction
A l_map2(l_m, true);
auto l_time2B(std::chrono::high_resolution_clock::now());

auto l_DurB(l_time2B - l_time2);
std::cout << std::chrono::duration_cast<std::chrono::microseconds>(l_DurB).count() << std::endl;
}