This is the second part in a series of articles checking the MuditaOS operating system. In this article, we cover the bottlenecks of the project that are worth refactoring. The PVS-Studio static analyzer and its optimization warnings will help us with this.
Recently, at one of the websites, where we posted the "Top 10 bugs found in C++ projects in 2021" article, someone asked the following questions in the comments:
How is PVS-Studio dealing with modern C++? Does the analyzer know how to detect incorrect use of move semantics and other insidious things of new C++ standards?
At that moment, I had an idea to write a separate article on micro-optimization diagnostics rules. Among these diagnostics, there are a lot of those that work with language constructs from the new language standards.
I note that the rules are called micro-optimizational for a reason. If you fix a couple of micro-optimization warnings, most often you won't get a noticeable performance gain. No one guarantees a crucial change in performance though. However, if you approach the issue comprehensively, you can often achieve significant improvements in project performance.
The most efficient way to increase performance is to use PVS-Studio together with some profiler. The static analyzer does not know how often the code fragment will be used, but simply says that some fragments should be rewritten in a more optimal way. It is the profiler that allows you to identify the most used code fragments. The method is as follows: to combine the output of both tools and, first of all, to fix the warnings of the static analyzer in the places pointed by the profiler.
In this article I will describe a lot of analyzer warnings. When evaluating them, I suggest taking a wider perspective and thinking about each of them as a small edit on the scale of a large code refactoring.
Since the project is regularly updated, to check it, I froze it in version 8cc1f77. So, without further delay, let's see what we managed to find!
V833 Passing the const-qualified object 'fileIndexerAudioPaths' to the 'std::move' function disables move semantics. BellHybridMain.cpp 77
int main()
{
const std::vector<std::string> fileIndexerAudioPaths = ....
....
std::vector<std::unique_ptr<sys::BaseServiceCreator>> systemServices;
....
systemServices.emplace_back(sys::CreatorFor<
service::ServiceFileIndexer>(std::move(fileIndexerAudioPaths)));
....
}Let's start with the diagnostic rule that we implemented in PVS-Studio 7.16. This rule says that a developer is trying to apply std::move to a constant object.
The code does not work as the developer expects: a constant object is not moved, because the std::move function does not actually move the object and does not guarantee that the object will move. With the use of static_cast, the std::move function simply casts the passed argument to the T&& type. Roughly speaking, when you call std::move, you are requesting a move, not directly telling the compiler to move the object. If want to know more details, we invite you to check the corresponding section of the knowledge base on our website — "move semantics".
In this case, the move will not be performed because we cannot modify the constant object. To fix this code, you can remove the 'const' keyword from the local variable:
int main()
{
std::vector<std::string> fileIndexerAudioPaths = ....
....
std::vector<std::unique_ptr<sys::BaseServiceCreator>> systemServices;
....
systemServices.push_back(sys::CreatorFor<
service::ServiceFileIndexer>(std::move(fileIndexerAudioPaths)));
....
}Or, if the 'const' keyword is important, it makes sense to remove the redundant std::move call:
int main()
{
const std::vector<std::string> fileIndexerAudioPaths = ....
....
std::vector<std::unique_ptr<sys::BaseServiceCreator>> systemServices;
....
systemServices.push_back(sys::CreatorFor<
service::ServiceFileIndexer>(fileIndexerAudioPaths));
....
}Also, as you may have noticed, we replaced the emplace_back function with push_back in the fixed code fragment. And we did it for reasons. The first reason is that emplace_back is a variadic function template in the std::vector class template. The compiler needs to additionally instantiate the function based on the arguments passed. More instantiations means to spend more time building the project. The second reason is that push_back is a function with two overloads in the std::vector class template.
But what about the special street magic of the emplace_back function, that allows you to create an object immediately in the vector? No magic here. In both cases, the container will request memory from the allocator to place the object. After that, the container will call the move constructor. You can find more information on this topic here.
The analyzer issued quite a few V833 warnings for the MuditaOS project. This is a new diagnostic rule, and I really like it, so let me show you a few more warnings:
V833 Passing the const-qualified object 'text' to the 'std::move' function disables move semantics. OptionBellMenu.hpp 30
class OptionBellMenu
{
public:
OptionBellMenu(const UTF8 &text, ....)
: text(std::move(text))
, ....
{
}
....
private:
UTF8 text;
....
}V833 Passing the const-qualified object 'blocks' to the 'std::move' function disables move semantics. TextDocument.cpp 13
class TextDocument
{
....
std::list<TextBlock> blocks;
....
}
....
TextDocument::TextDocument(const std::list<TextBlock> &blocks)
: blocks(std::move(blocks))
{
}The approach to fix these errors is similar to the way we fixed the first error. So I don't see any point in repeating myself. In total, the analyzer found about 20 V833 warnings in the project.
Now let's look at another warning related to move semantics:
V820 The 'snoozed' variable is not used after copying. Copying can be replaced with move/swap for optimization. AlarmPresenter.cpp 27
void AlarmPopupContract::AlarmModel::setSnoozed(
std::vector<SingleEventRecord> snoozed)
{
this->snoozedRecord = snoozed;
}The analyzer has detected code fragment where a variable is copied to another variable but is never used after that. Such code can be optimized by removing the unnecessary copy operation. For example, use the std::move function:
void AlarmPopupContract::AlarmModel::setSnoozed(
std::vector<SingleEventRecord> snoozed)
{
this->snoozedRecord = std::move(snoozed);
}In total, the analyzer issued around 40 warnings of this diagnostic. Here are a few of them:
V830 Decreased performance. Consider replacing the 'draft.value()' expression to '*draft'. SMSInputWidget.cpp 158
class SMSInputWidget : public ListItem
{
....
std::optional<SMSRecord> draft;
....
}
....
void SMSInputWidget::updateDraftMessage(....)
{
....
if (draft.has_value())
{
app->updateDraft(draft.value(), inputText);
}
....
}Here, while calling has_value, we see that the draft variable (whose type os std::optional)definitely contains a value inside. In this case, you don't need to call the value() method that will check again if there is a value before returning it. Use the * operator that will return the value that is obviously available here.
Here, one could argue that modern compilers optimize such code quite well. Yes, it would be correct to call this fix as code optimization that possibly reduces the overhead. If the compiler can't substitute function bodies (inlining) or such optimization is disabled, then the code version proposed below will work faster, and in other cases at least not slower:
void SMSInputWidget::updateDraftMessage(....)
{
....
if (draft.has_value())
{
app->updateDraft(*draft, inputText);
}
....
}Here is another similar code example:
void ThreadItem::setContactName(std::optional<long int> numberImportance)
{
....
if (numberImportance.has_value())
{
displayNumberImportance(numberImportance.value());
}
....
}You can refactor the code as follows:
void ThreadItem::setContactName(std::optional<long int> numberImportance)
{
....
if (numberImportance.has_value())
{
displayNumberImportance(*numberImportance);
}
....
}However, such fixes have a downside: if you look at the use of the * overloaded operator and do not see the variable declaration, you might think that you are dealing with a pointer. Many people think that this is a rather strange semantics that should not be used. If you are one of these people, you can be easily disable this rule.
Just as with the V833 diagnostic, the analyzer issued a lot of similar V830 warnings (66 in total). If I decided to list them, it would take quite a few pages. So, let me show you just a few of them:
V827 Maximum size of the 'actions' vector is known at compile time. Consider pre-allocating it by calling actions.reserve(3). BellAlarmHandler.cpp
auto BellAlarmClockHandler::getActions(sys::Service *service) -> Actions
{
Actions actions;
actions.emplace_back(....);
actions.emplace_back(....);
actions.emplace_back(....);
return actions;
}Here, we see the vector whose size is known at compile time. The analyzer suggests calling the reserve function before filling in the vector. If you don't call the reserve function, the emplace_back calls can lead to the internal buffer reallocation in the vector and the movement of elements to a new memory area. And if the move constructor of a class whose objects are stored in a vector is not marked as noexcept, the vector won't move, but copy the objects. You can reduce the overhead by allocating a buffer of the appropriate size. Here is the correct code:
auto BellAlarmClockHandler::getActions(sys::Service *service) -> Actions
{
Actions actions;
Actions.reserve(3);
actions.emplace_back(....);
actions.emplace_back(....);
actions.emplace_back(....);
return actions;
}By the way, do you always make sure to mark your user-provided move constructors/operators as noexcept?
Traditionally, for MuditaOS, we have received many warnings of this diagnostic. Before we look at another V827 diagnostic warning, we need to explain some details of how this diagnostic works.
The diagnostic rule works based on the data flow analysis mechanism and suggests reserving the maximum possible number of elements. That is, if an element is added under a condition, the analyzer will take it into account and offer to reserve the maximum possible container size.
Let's look at a similar example:
V827 Maximum size of the 'ret' vector is known at compile time. Consider pre-allocating it by calling ret.reserve(8). Commands.cpp 11
std::vector<AT> getCommadsSet(commadsSet set)
{
std::vector<AT> ret;
switch (set)
{
case commadsSet::modemInit:
ret.push_back(AT::URC_NOTIF_CHANNEL);
ret.push_back(AT::RI_PIN_AUTO_CALL);
ret.push_back(AT::RI_PIN_PULSE_SMS);
ret.push_back(AT::RI_PIN_PULSE_OTHER);
ret.push_back(AT::URC_DELAY_ON);
ret.push_back(AT::URC_UART1);
ret.push_back(AT::AT_PIN_READY_LOGIC);
ret.push_back(AT::CSQ_URC_ON);
break;
case commadsSet::simInit:
ret.push_back(AT::CALLER_NUMBER_PRESENTATION);
ret.push_back(AT::SMS_TEXT_FORMAT);
ret.push_back(AT::SMS_GSM);
ret.push_back(AT::CRC_ON);
break;
case commadsSet::smsInit:
ret.push_back(AT::SET_SMS_STORAGE);
ret.push_back(AT::SMS_TEXT_FORMAT);
ret.push_back(AT::SMS_GSM);
break;
}
return ret;
}According to code, 8 push_back function may be called in the longest of the switch operator branches. The analyzer, detecting this, suggests invoking ret.reserve(8).
Here is the list of a few more V827 triggerings:
Let's now move on to the next innovative diagnostic. The diagnostic detects containers from the standard library that you can replace with other containers for optimization purposes.
To determine which container type will suit better in a given case, heuristics are used based on what operations are performed on the container. The analyzer also calculates algorithmic complexity of all the operations and suggests a container whose algorithmic complexity is lowest. Let's see what we found with the help of this diagnostic:
V826 Consider replacing the 'dbFileExt' std::vector with std::array. The size is known at compile time. common.cpp 9
void RemoveDbFiles(const std::string &dbName)
{
std::vector<std::string> dbFileExt = {".db", ".db-journal", ".db-wal"};
for (const auto &ext : dbFileExt)
{
const auto dbPath = (std::filesystem::path{"sys/user"} /
std::filesystem::path{dbName + ext});
if (std::filesystem::exists(dbPath))
{
std::filesystem::remove(dbPath.c_str());
}
}
}In this case, the analyzer says that with the container size known at compile time. It is preferable to use std::array instead of std::vector. It will help avoid dynamic allocation. You can also do the following:
The function after all fixes looks as follows:
void RemoveDbFiles(const std::string &dbName)
{
using namespace std::literals;
static constexpr std::array dbFileExt =
{".db"sv, ".db-journal"sv, ".db-wal"sv};
for (auto ext : dbFileExt)
{
const auto dbPath = (std::filesystem::path{"sys/user"} /
std::filesystem::path{dbName + std::string { ext }});
if (std::filesystem::exists(dbPath))
{
std::filesystem::remove(dbPath.c_str());
}
}
}You can compare the output generated by the GCC compiler for the original and optimized code on Compiler Explorer.
In general, the application field of the V826 diagnostic rule is wide and covers many different cases. Here is another example of triggering:
V826 Consider replacing the 'carriers' std::list with std::vector. Contiguous placement of elements in memory can be more efficient. SpecialInputModel.cpp 45
void SpecialInputModel::buildGrid(const std::vector<char32_t> &elements)
{
while (....)
{
....
std::list<gui::Carrier> carriers;
for (....)
{
....
carriers.push_back(....);
....
carriers.push_back(....);
}
....
}
....
internalData.push_back
(new gui::SpecialInputTableWidget(...., std::move(carries));
}This warning is, of course, controversial. That's why the analyzer gives it the third level of severity. It is due to the fact that a developer adds elements only to the end of the container, as it usually happens with std::vector.
Whether MuditaOS developers should fix it or not, as I've already said, is a moot point:
This is just a couple of all V826 diagnostic warnings that seemed interesting to me. In reality, the analyzer issued much more warnings. Some of these warnings are very easy to fix. For example, like in the case when the container is locally created, used and destroyed after exiting the function. Other warnings are more complicated. Like those where the container is traversed through several functions.
As in the previous case, I am not sure which warnings should be fixed and which ones should not. So I'll leave it to the MuditaOS developers. Meanwhile, we are moving on!
Usually, warnings about unused variables are not so gripping. When you're reading code, you can't be sure that the error found indicates an incorrectly implemented algorithm or that the code does not work as developer expected it to work. Rather, it seems that the troublesome code was changed during refactoring, and somebody simply forgot to delete the unused variable.
When looking through the log with diagnostic warnings, I found one interesting code pattern for which the analyzer complained:
V808 'valStr' object of 'basic_string' type was created but was not utilized. AlarmSettingsModel.cpp 23
void AlarmVolumeModel::setValue(std::uint8_t value)
{
const auto valStr = std::to_string(value);
audioModel.setVolume(value, AbstractAudioModel::PlaybackType::Alarm, {});
}We found similar code fragment 12 lines below:
V808 'valStr' object of 'basic_string' type was created but was not utilized. PrewakeUpSettingsModel.cpp 35
void PrewakeUpChimeVolumeModel::setValue(std::uint8_t value)
{
const auto valStr = std::to_string(value);
audioModel.setVolume(value, AbstractAudioModel::PlaybackType::PreWakeup, {});
}And a couple more warnings issued on the same code pattern:
Several warnings issued on the same code pattern give me philosophical thoughts. Firstly, the code was definitely copy-pasted. Secondly, the unused variable indicates that the code was definitely rewritten. I wonder which of these things happened earlier...
Here are a few more V808s:
V817 It is more efficient to seek '/' character rather than a string. TagsFetcher.cpp 28
std::optional<Tags> fetchTagsInternal(std::string filePath)
{
....
if (const auto pos = filePath.rfind("/"); pos == std::string::npos)
{
....
}
....
}The analyzer detected the code fragment that looks for a character in a string. The fragment can be optimized. You can use the find overload that receives a character instead of a string. Searching for a substring in a string means going through all the characters in the strings — two loops. If we are searching for a character, we need to go through one loop. Optimized version:
std::optional<Tags> fetchTagsInternal(std::string filePath)
{
....
if (const auto pos = filePath.rfind('/'); pos == std::string::npos)
{
....
}
....
}Here are a few more warnings to pay attention to:
Next, let's look at the warnings that indicate inefficient calculation of the string length:
V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. ATStream.cpp 127
constexpr auto delimiter = "\r\n"
....
void ATStream::countLines()
{
....
auto pos = ....;
while (pos != std::string::npos)
{
if ((lastPos) != pos)
{
....
}
lastPos = pos + std::strlen(at::delimiter);
}
}The analyzer has detected a situation in which each loop's iteration calls the std::strlen function with the delimiter constant. The value of the constant isn't changed. It means that the string length can be calculated beforehand. This optimizes the code. Let's use C++17 and change the constant type to std::string_view. We can get the string length with O(1) by calling the size non-static member function:
constexpr std::string_view delimiter = "\r\n"
....
void ATStream::countLines()
{
....
auto pos = ....;
auto delimiterLen = delimiter.size();
while (pos != std::string::npos)
{
if ((lastPos) != pos)
{
....
}
lastPos = pos + delimiterLen;
}
}Here is another similar case:
V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. DLCChannel.cpp 140
This is not all the "adventures" of the delimiter constant. The analyzer issued a couple of warnings for another function:
V810 Decreased performance. The 'std::strlen(at::delimiter)' function was called several times with identical arguments. The result should possibly be saved to a temporary variable, which then could be used while calling the 'substr' function. ATStream.cpp 89
V811 Decreased performance. Excessive type casting: string -> char * -> string. Consider inspecting the expression. ATStream.cpp 89
bool ATStream::checkATBegin()
{
auto pos = atBuffer.find(at::delimiter, std::strlen(at::delimiter));
....
std::string rr = atBuffer.substr(std::strlen(at::delimiter),
pos - std::strlen(at::delimiter)).c_str();
....
}Let's fix both code fragments. Remember that after the fix from the previous example, delimiter is now std::string_view:
bool ATStream::checkATBegin()
{
auto delimiterLen = delimiter.size();
auto pos = atBuffer.find(at::delimiter, delimiterLen);
....
std::string rr = atBuffer.substr(delimiterLen
pos - delimiterLen);
....
}Here are similar warnings of V810 and V811 diagnostics, which are worth paying attention to:
V821 [CERT-DCL19-C] Decreased performance. The 'factory' variable can be constructed in a lower level scope. CallLogDetailsWindow.cpp 147
void CallLogDetailsWindow::initNumberWidget()
{
....
ActiveIconFactory factory(this->application);
....
if (....)
{
....
}
else
{
....
numberHBox->addIcon(factory.makeCallIcon(numberView));
numberHBox->addIcon(factory.makeSMSIcon(numberView));
....
}
}The analyzer has detected the factory variable that could be created in a lower level scope. By changing the scope of an object, you can optimize the code's performance and memory consumption.
The correct version of the code may look like this:
void CallLogDetailsWindow::initNumberWidget()
{
....
if (....)
{
....
}
else
{
....
ActiveIconFactory factory(this->application);
numberHBox->addIcon(factory.makeCallIcon(numberView));
numberHBox->addIcon(factory.makeSMSIcon(numberView));
....
}
}The analyzer issued V821 diagnostic warnings for several more code fragments. Here is the list of them:
Strangely enough, we covered only part of the micro-optimization diagnostic warnings found in MuditaOS. In fact, there are about a thousand of them. I think this article is already lengthy enough and, if I show you more warnings, it will be just hard to read.
As I said at the beginning of the article, if you fix micro-optimization warnings one at a time, it will most likely not greatly affect the performance of the whole project. However, if you fix all of them, or at least most of them, sometimes you can get a noticeable performance gain. But, of course, it depends more on the case, or rather, on how often inefficient code fragments are executed.
One day, at a conference, one of our clients stopped by our stand. They told us that his team increased the project performance by tens of percent by using PVS-Studio. They simply fixed several troublesome functions that for some reason took a vector of strings not by reference but by value. Unfortunately, there are no proofs.
If after reading this article you have a desire to check your project, you can easily do this by requesting a trial key on our website. If you already use PVS-Studio and have not used optimization diagnostics before — this is exactly the right time to try them out.