Это вторая часть цикла статей про проверку операционной системы MuditaOS. В ней мы поговорим об узких местах в проекте, которые стоит отрефакторить. В этом нам поможет статический анализатор PVS-Studio и его оптимизационные предупреждения.
Недавно на одном из сайтов, где была опубликована статья про Топ-10 C и С++ ошибок, найденных PVS-Studio в 2021 году, в комментариях спросили примерно следующее:
Как у PVS-Studio обстоят дела с современным С++? Умеет ли анализатор находить неправильное использование move-семантики и прочие коварства новых стандартов С++?
В этот момент у меня и возникла идея написать отдельную статью по микрооптимизационным диагностикам. Среди них как раз много тех, которые работают с языковыми конструкциями из новых стандартов языка.
Отмечу, что правила называются микрооптимизационными не зря. Правка нескольких срабатываний анализатора чаще всего не даст существенного ускорения, да оно и не гарантируется. Однако если подойти к вопросу комплексно, то зачастую можно добиться существенных улучшений производительности проекта.
Особенно эффективно можно сделать это, если использовать PVS-Studio в паре с каким-нибудь профилировщиком. Статический анализатор не знает, как часто будет использоваться фрагмент кода, а просто говорит, что некоторые места можно переписать более оптимальным образом. Профилировщик же как раз позволяет выявить наиболее используемые участки. Суть метода в том, чтобы соединить их вывод и в первую очередь править срабатывания статического анализатора в местах, на которые указывает профилировщик.
В данной статье я приведу довольно много срабатываний. При их оценке предлагаю смотреть шире и думать о каждом из них как о маленькой правке в масштабах большого рефакторинга кода.
Так как проект регулярно обновляется, для проверки я зафиксировал его версией 8cc1f77. Итак, предлагаю больше не тянуть и наконец посмотреть, что же нам удалось найти!
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)));
....
}Пожалуй, начнём с диагностического правила, вышедшего в недавнем релизе PVS-Studio 7.16. Данное правило говорит о том, что std::move пытаются применить на константный объект.
Код работает не так, как ожидает программист: перемещение константного объекта не произойдёт, ведь функция std::move на самом деле не перемещает объект и не гарантирует, что он переместится. Она просто преобразует переданный аргумент к типу T&& при помощи static_cast. Грубо говоря, вызов std::move – это запрос на перемещение, а не прямое указание компилятору переместить объект. Более подробно данная тема раскрыта в базе знаний на нашем сайте в разделе "семантика перемещения".
В данном случае перемещение не выполнится просто потому, что мы не можем модифицировать константный объект. Поправить фрагмент кода можно, убрав константность с локальной переменной:
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)));
....
}Или, если константность критична, есть смысл удалить избыточный вызов std::move:
int main()
{
const std::vector<std::string> fileIndexerAudioPaths = ....
....
std::vector<std::unique_ptr<sys::BaseServiceCreator>> systemServices;
....
systemServices.push_back(sys::CreatorFor<
service::ServiceFileIndexer>(fileIndexerAudioPaths));
....
}Также, как вы могли заметить, в исправлениях функция emplace_back была заменена на push_back. Сделано это было неспроста. Первая причина: - это вариативный шаблон функции в шаблоне класса std::vector. Компилятору придется её дополнительно инстанцировать на основе переданных аргументов. Больше инстанцирований – больше проводим времени за сборкой проекта. Вторая причина: - это обычная функция с двумя перегрузками в шаблоне класса std::vector.
А как же особая уличная магия функции emplace_back, которая позволяет создать объект сразу в векторе? А магии не произойдёт. В обоих случаях контейнер запросит у аллокатора память для размещения объекта, а затем вызовет конструктор перемещения. Подробнее с данной темой можно ознакомиться тут.
На проекте MuditaOS было обнаружено довольно много срабатываний V833. Это новое диагностическое правило, и оно мне очень нравится, поэтому приведу ещё несколько предупреждений:
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))
{
}Принцип исправления данных ошибок схож с тем, как мы исправили первую, поэтому не вижу смысла повторяться. В общей сложности в проекте было найдено около 20 срабатываний V833.
Теперь давайте рассмотрим ещё одно предупреждение, связанное с move-семантикой:
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;
}Анализатор обнаружил код, в котором переменная не используется после того, как её скопировали в другую переменную. Данный фрагмент кода можно оптимизировать, убрав одно из копирований. Это можно сделать, например, применив функцию std::move:
void AlarmPopupContract::AlarmModel::setSnoozed(
std::vector<SingleEventRecord> snoozed)
{
this->snoozedRecord = std::move(snoozed);
}Всего анализатор выдал в районе 40 предупреждений данных диагностик. Приведу несколько из них:
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);
}
....
}Здесь при помощи проверки has_value мы видим, что внутри переменной draft, тип которой std::optional, точно есть значение. В таком случае можно не звать метод value(), который перед возвращением значения ещё раз проверит, есть ли оно, а воспользоваться оператором *, который вернёт заведомо имеющееся в данном случае значение.
Тут можно было бы возразить, что современные компиляторы неплохо оптимизируют такой код. Да, эту правку правильно было бы назвать оптимизацией с целью возможного уменьшения накладных расходов. Если у компилятора нет возможности подставлять тела функций (инлайнинг) или жёстко отключена такая оптимизация, то предложенный ниже вариант кода будет работать быстрее, а в остальных случаях как минимум не медленнее:
void SMSInputWidget::updateDraftMessage(....)
{
....
if (draft.has_value())
{
app->updateDraft(*draft, inputText);
}
....
}Приведу ещё один похожий пример кода:
void ThreadItem::setContactName(std::optional<long int> numberImportance)
{
....
if (numberImportance.has_value())
{
displayNumberImportance(numberImportance.value());
}
....
}Отрефакторить его можно таким образом:
void ThreadItem::setContactName(std::optional<long int> numberImportance)
{
....
if (numberImportance.has_value())
{
displayNumberImportance(*numberImportance);
}
....
}Однако у подобных правок есть минус: если посмотреть на использование перегруженного оператора * и не видеть декларации переменной, то можно подумать, что ведётся работа с указателем. Есть много людей, считающих, что это довольно странная семантика, которую не стоит использовать. Если вы из их числа, то данное правило легко можно отключить.
Так же, как и с предупреждениями диагностики V833, было выдано довольно много подобных срабатываний (всего 66). Если бы я решил привести их в виде списка, это заняло бы довольно много места. Поэтому приведу лишь несколько из них:
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;
}Здесь мы видим вектор с известным на этапе компиляции размером. Анализатор предлагает вызвать функцию reserve перед его заполнением. В случае если этого не сделать, вызовы emplace_back могут приводить к реаллокации внутреннего буфера в векторе и перемещению элементов в новую область памяти. А если конструктор перемещения класса, объекты которого хранятся в векторе, не помечен как noexcept, то вектор будет не перемещать, а копировать объекты. Уменьшить накладные расходы можно, как раз выделив буфер нужного размера. Исправленный вариант кода:
auto BellAlarmClockHandler::getActions(sys::Service *service) -> Actions
{
Actions actions;
Actions.reserve(3);
actions.emplace_back(....);
actions.emplace_back(....);
actions.emplace_back(....);
return actions;
}Кстати, а вы не забываете помечать ваши пользовательские конструкторы/операторы перемещения как noexcept?
Традиционно для MuditaOS мы получили много срабатываний данной диагностики. Перед тем, как мы рассмотрим ещё одно предупреждение V827, нужно пояснить некоторые детали работы этой диагностики.
Правило работает на основе механизма анализа потока данных и предлагает зарезервировать максимально возможное количество элементов. То есть если элемент добавляется под условием, анализатор учтёт его и в итоге предложит зарезервировать максимально возможный размер контейнера.
Давайте рассмотрим подобный пример:
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;
}Как видно по коду, в самой длинной из веток оператора switch будет совершено 8 вызовов функции push_back. Анализатор, видя это, предлагает сделать ret.reserve(8).
Приведу ещё несколько срабатываний V827 в виде списка:
Давайте теперь перейдём к следующей инновационной диагностике. Она обнаруживает контейнеры из стандартной библиотеки, которые можно заменить на другие в целях оптимизации.
Чтобы понять, какой именно контейнер лучше подойдет в конкретном случае, применяется эвристика, основанная на типах операций, которые производят с контейнером. Кроме этого, в некоторых случаях анализатор вычисляет алгоритмическую сложность всех операций и предлагает контейнер, у которого она будет ниже. Давайте же посмотрим, что нам удалось найти при помощи этой диагностики:
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());
}
}
}В данном случае анализатор говорит о том, что при известном на этапе компиляции размере контейнера, вместо std::vector предпочтительнее использовать std::array. Это позволит избежать накладных расходов в виде динамических аллокаций. Также можно сделать следующее:
Функция после всех правок выглядит следующим образом:
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());
}
}
}Здесь можно сравнить вывод, сгенерированный компилятором GCC для оригинального и оптимизированного кода на Compiler Explorer.
Вообще, область применения диагностического правила V826 обширна и покрывает много разных случаев, вот ещё один пример срабатывания:
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));
}Данное срабатывание, конечно, спорное. Поэтому анализатор помещает его на третий уровень достоверности. Обусловлено оно тем, что программист добавляет элементы только в конец контейнера, как это обычно происходит с std::vector.
Править его разработчикам MuditaOS или нет, как я уже говорил, вопрос спорный:
Это всего пара из всех показавшихся мне интересными случаев срабатывания диагностики V826. В реальности их гораздо больше. Часть из них, как случай с локально созданным, использованным и уничтоженным по выходу из функции контейнером, поправить очень легко. Другие, в которых контейнер пробрасывается через несколько функций, уже посложнее.
Я, как и в прошлом случае, не очень уверен, какие из них стоит править, а какие нет, поэтому также оставлю их на откуп разработчикам. А мы тем временем двигаемся дальше!
Обычно срабатывания о неиспользуемых переменных не очень интересны. Просто читая код, нельзя с уверенностью сказать, что найденная ошибка говорит о неправильно реализованном алгоритме или что код работает не так, как ожидал программист. Скорее, создаётся ощущение, что проблемный участок кода изменили в ходе рефакторинга, а неиспользуемую больше переменную просто забыли удалить.
Просматривая лог с такими предупреждениями, я обнаружил один интересный паттерн, на который данная диагностика ругалась:
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, {});
}Похожий фрагмент кода встретился на 12 строк ниже:
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, {});
}И ещё пара срабатываний анализатора на такой же паттерн кода:
Несколько срабатываний на одинаковые фрагменты кода наводят на философские мысли. Во-первых, код определённо скопипастили. Во-вторых, неиспользуемая переменная говорит о том, что код определённо переписывали. Интересно, что из этого произошло раньше...
Приведу ещё несколько V808. Традиционно, их гораздо больше:
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)
{
....
}
....
}Анализатор обнаружил фрагмент кода, занимающийся поиском символа в строке. Этот код можно оптимизировать, используя перегрузку find, принимающую символ вместо строки. Поиск подстроки в строке – это перебор всех символов в них – два цикла. Если же мы ищем символ, то мы можем обойтись одним. Оптимизированный код:
std::optional<Tags> fetchTagsInternal(std::string filePath)
{
....
if (const auto pos = filePath.rfind('/'); pos == std::string::npos)
{
....
}
....
}Вот ещё несколько предупреждений, на которые стоит обратить внимание:
Далее рассмотрим предупреждения, говорящие о неэффективном вычислении размера строки:
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);
}
}Анализатор обнаружил ситуацию, в которой при каждой итерации цикла вызывается функция std::strlen с константой delimiter. Значение константы не меняется, а значит длину строки можно вычислить заранее и этим оптимизировать код. Воспользуемся C++17 и переделаем тип константы на std::string_view. Размер теперь можно будет получить за O(1), вызвав функцию-член size:
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;
}
}Также анализатор выдал срабатывание на ещё один похожий случай:
V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. DLCChannel.cpp 140
Это ещё не все приключения константы delimiter. Анализатор выдал пару срабатываний на другую функцию:
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();
....
}Давайте поправим оба срабатывания. Помним, что после исправления из предыдущего примера, delimiter теперь 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);
....
}Похожие срабатывания диагностик V810 и V811, на которые стоит обратить внимание:
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));
....
}
}Здесь анализатор сообщает о том, что переменная factory могла бы быть создана в меньшей области видимости. Изменив область её создания, можно оптимизировать код по скорости и количеству потребляемой памяти.
Пример исправленного кода:
void CallLogDetailsWindow::initNumberWidget()
{
....
if (....)
{
....
}
else
{
....
ActiveIconFactory factory(this->application);
numberHBox->addIcon(factory.makeCallIcon(numberView));
numberHBox->addIcon(factory.makeSMSIcon(numberView));
....
}
}Анализатор выдал предупреждения V821 ещё на несколько фрагментов кода. Приведу их в виде списка:
Как ни странно, это была только часть микрооптимизационных срабатываний, обнаруженных на MuditaOS. На деле же их около тысячи. Мне кажется, что статья итак довольно сильно разрослась и, если привести ещё больше предупреждений, её будет просто тяжело читать.
Как я уже говорил в начале статьи, правка микрооптимизационных срабатываний по одному, скорее всего, не сильно повлияет на производительность целого проекта. Однако если поправить их все или хотя бы большую часть, то иногда можно добиться заметного прироста производительности. Но тут, конечно, всё больше зависит от случая, а вернее, от того, как часто выполняются неэффективные участки кода.
Однажды на конференции посетитель, заглянувший к нам на стенд, рассказал, что его команда с помощью PVS-Studio увеличила скорость работы проекта на десятки процентов. Для этого они просто исправили несколько неудачных функций, которые почему-то принимали в качестве аргумента не ссылку на вектор строк, а вектор строк по значению. К сожалению, proof-ов не будет.
Если после прочтения данной статьи у вас появилось желание проверить свой проект, то это можно легко сделать, получив пробный ключ у нас на сайте. Если вы уже пользуетесь PVS-Studio и до этого не использовали оптимизационные диагностики – то это отличный случай, чтобы попробовать их в деле.