Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
MuditaOS: Зазвонит ли ваш будильник? Ча…

MuditaOS: Зазвонит ли ваш будильник? Часть 2

21 Фев 2022

Это вторая часть цикла статей про проверку операционной системы MuditaOS. В ней мы поговорим об узких местах в проекте, которые стоит отрефакторить. В этом нам поможет статический анализатор PVS-Studio и его оптимизационные предупреждения.

0925_MuditaOS_2_ru/image1.png

Введение

Недавно на одном из сайтов, где была опубликована статья про Топ-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 предупреждений данных диагностик. Приведу несколько из них:

  • V833 Passing the const-qualified object 'result->snoozedAlarms' to the 'std::move' function disables move semantics. ActiveNotificationsModel.cpp 213
  • V833 Passing the const-qualified object 'scheme' to the 'std::move' function disables move semantics. ColorTestWindow.cpp 79
  • V833 Passing the const-qualified object 'text' to the 'std::move' function disables move semantics. OptionsWidgetMaker.cpp 17
  • ....
  • V820 The 'dayMonthText' variable is not used after copying. Copying can be replaced with move/swap for optimization. CalendarData.hpp 51
  • V820 The 'newRange' variable is not used after copying. Copying can be replaced with move/swap for optimization. SpinnerPolicies.hpp 83
  • V820 The 'newRange' variable is not used after copying. Copying can be replaced with move/swap for optimization. SpinnerPolicies.hpp 290
  • ....

Работа с std::optional

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). Если бы я решил привести их в виде списка, это заняло бы довольно много места. Поэтому приведу лишь несколько из них:

  • V830 Decreased performance. Consider replacing the 'lastSms.value()' expression to '*lastSms'. NewMessage.cpp 358
  • V830 Decreased performance. Consider replacing the 'currentFileToken.value()' expression to '*currentFileToken'. SongsPresenter.cpp 69
  • V830 Decreased performance. Consider replacing the 'returnedContact.value()' expression to '*returnedContact'. PhonebookNewContact.cpp 171
  • V830 Decreased performance. Consider replacing the 'activeDevice.value()' expression to '*activeDevice'. BluetoothSettingsModel.cpp 94
  • V830 Decreased performance. Consider replacing the 'selectedDevice.value()' expression to '*selectedDevice'. AllDevicesWindow.cpp 75
  • V830 Decreased performance. Consider replacing the 'blockSizeConstraint.value()' expression to '*blockSizeConstraint'. StreamFactory.cpp 72
  • ....

Контейнеры STL

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 в виде списка:

  • V827 Maximum size of the 'data' vector is known at compile time. Consider pre-allocating it by calling data.reserve(3) ServiceCellular.cpp 1093
  • V827 Maximum size of the 'commandParts' vector is known at compile time. Consider pre-allocating it by calling commandParts.reserve(8) CallForwardingRequest.cpp 42
  • V827 Maximum size of the 'pathElements' vector is known at compile time. Consider pre-allocating it by calling pathElements.reserve(4) AudioCommon.cpp 51

Давайте теперь перейдём к следующей инновационной диагностике. Она обнаруживает контейнеры из стандартной библиотеки, которые можно заменить на другие в целях оптимизации.

Чтобы понять, какой именно контейнер лучше подойдет в конкретном случае, применяется эвристика, основанная на типах операций, которые производят с контейнером. Кроме этого, в некоторых случаях анализатор вычисляет алгоритмическую сложность всех операций и предлагает контейнер, у которого она будет ниже. Давайте же посмотрим, что нам удалось найти при помощи этой диагностики:

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. Это позволит избежать накладных расходов в виде динамических аллокаций. Также можно сделать следующее:

  • Объявить массив со спецификатором static, чтоб он вычислялся один раз.
  • Раз внутрь контейнера кладутся строковые литералы, то следует заменить std::string на std::string_view. Так как ниже в коде используется библиотека filesystem, то можно сделать предположение, что код собирается с версией стандарта C++17 и std::string_view в кодовой базе также может использоваться.
  • Хм, теперь у нас массив std::string_view, оба класса умеют работать в compile-time. Значит, можно объявить массив со спецификатором constexpr.

Функция после всех правок выглядит следующим образом:

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 или нет, как я уже говорил, вопрос спорный:

  • С одной стороны, они создают std::list, добавляют в него элементы и пробрасывают его дальше. В этом узком случае лучше std::list, так как добавление элементов в конец гарантированно происходит за константное время. Добавление элементов в вектор происходит за амортизированное константное время вследствие возможных реаллокаций, свойственных этому контейнеру.
  • С другой стороны, элементы добавляются не просто так. Уже в функции SpecialInputTableWidget контейнер carriers обходится. В этом случае предпочтительнее использовать std::vector. Контейнер std::list не обязан располагать данные последовательно, вследствие чего при его обходе возможны промахи кэша. Вектор же, благодаря последовательному расположению элементов в памяти, гораздо более дружелюбен к кэшу процессора. Это даёт выигрыш при линейном обращении к его элементам, если размер элементов невелик. Чем меньше размер элементов по сравнению с кэш-линией, тем больше элементов процессор может прогрузить за одно чтение.

Это всего пара из всех показавшихся мне интересными случаев срабатывания диагностики 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 'valStr' object of 'basic_string' type was created but was not utilized. SnoozeSettingsModel.cpp 76
  • V808 'valStr' object of 'basic_string' type was created but was not utilized. BedtimeModel.cpp 80

Несколько срабатываний на одинаковые фрагменты кода наводят на философские мысли. Во-первых, код определённо скопипастили. Во-вторых, неиспользуемая переменная говорит о том, что код определённо переписывали. Интересно, что из этого произошло раньше...

Приведу ещё несколько V808. Традиционно, их гораздо больше:

  • V808 'deviceAddress' object of 'basic_string' type was created but was not utilized. A2DP.cpp 332
  • V808 'operatorNames' object of 'vector' type was created but was not utilized. NetworkSettings.cpp 263
  • V808 'volume' object of 'optional' type was created but was not utilized. AudioServiceAPI.cpp 224
  • ....

Строки

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) 
  {
    ....
  }
  ....
}

Вот ещё несколько предупреждений, на которые стоит обратить внимание:

  • V817 It is more efficient to seek '\"' character rather than a string. response.cpp 489
  • V817 It is more efficient to seek '\"' character rather than a string. ATURCStream.cpp 45
  • V817 It is more efficient to seek '\"' character rather than a string. ATURCStream.cpp 78
  • V817 It is more efficient to seek '.' character rather than a string. DatabaseInitializer.cpp 97
  • V817 It is more efficient to seek '.' character rather than a string. DbInitializer.cpp 87
  • V817 It is more efficient to seek ' ' character rather than a string. test-gui-TextBlockCursor.cpp 424
  • V817 It is more efficient to seek '+' character rather than a string. CallForwardingRequest.cpp 82
  • V817 It is more efficient to seek ',' character rather than a string. ServiceCellular.cpp 1398
  • V817 It is more efficient to seek 'a' character rather than a string. unittest_utf8.cpp 108

Далее рассмотрим предупреждения, говорящие о неэффективном вычислении размера строки:

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();
  ....
}
  • Первое срабатывание говорит о том, что функция strlen вызывается два раза в пределах одного выражения.
  • Второе срабатывание – о том, что в коде происходит что-то странное. Мы вызываем функцию substr у переменной atBuffer, она возвращает нам std::string. Далее мы вызываем у результата функцию c_str(), которая преобразует результат в const char*. После мы опять неявно преобразуем результат в std::string (считаем размер для строки, тип, который сейчас const char*, а значит, ещё один вызов strlen), и, наконец, присваиваем его переменной rr.

Давайте поправим оба срабатывания. Помним, что после исправления из предыдущего примера, 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, на которые стоит обратить внимание:

  • 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 106
  • V810 Decreased performance. The 'translate_mode_to_attrib(mode)' 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 'f_chmod' function. filesystem_vfat.cpp 560
  • V810 Decreased performance. The 'translate_mode_to_attrib(mode)' 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 'f_chmod' function. filesystem_vfat.cpp 572
  • V811 Decreased performance. Excessive type casting: string -> char * -> string. Consider inspecting the 'ss.str().c_str()' expression. AppMessage.hpp 216
  • V811 Decreased performance. Excessive type casting: string -> char * -> string. Consider inspecting the expression. ATStream.cpp 105
  • V811 Decreased performance. Excessive type casting: string -> char * -> string. Consider inspecting second argument of the function handleStart. ServiceAudio.cpp 73

Прочее

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 ещё на несколько фрагментов кода. Приведу их в виде списка:

  • V821 [CERT-DCL19-C] Decreased performance. The 'size' variable can be constructed in a lower level scope. BoxLayoutSizeStore.cpp 19
  • V821 [CERT-DCL19-C] Decreased performance. The 'local_style' variable can be constructed in a lower level scope. RichTextParser.cpp 385
  • V821 [CERT-DCL19-C] Decreased performance. The 'defaultValue' variable can be constructed in a lower level scope. ServiceAudio.cpp 702
  • V821 [CERT-DCL19-C] Decreased performance. The 'js' variable can be constructed in a lower level scope. i18n.cpp 84
  • V821 [CERT-DCL19-C] Decreased performance. The 'it' variable can be constructed in a lower level scope. disk_manager.cpp 49

Заключение

Как ни странно, это была только часть микрооптимизационных срабатываний, обнаруженных на MuditaOS. На деле же их около тысячи. Мне кажется, что статья итак довольно сильно разрослась и, если привести ещё больше предупреждений, её будет просто тяжело читать.

Как я уже говорил в начале статьи, правка микрооптимизационных срабатываний по одному, скорее всего, не сильно повлияет на производительность целого проекта. Однако если поправить их все или хотя бы большую часть, то иногда можно добиться заметного прироста производительности. Но тут, конечно, всё больше зависит от случая, а вернее, от того, как часто выполняются неэффективные участки кода.

Однажды на конференции посетитель, заглянувший к нам на стенд, рассказал, что его команда с помощью PVS-Studio увеличила скорость работы проекта на десятки процентов. Для этого они просто исправили несколько неудачных функций, которые почему-то принимали в качестве аргумента не ссылку на вектор строк, а вектор строк по значению. К сожалению, proof-ов не будет.

Если после прочтения данной статьи у вас появилось желание проверить свой проект, то это можно легко сделать, получив пробный ключ у нас на сайте. Если вы уже пользуетесь PVS-Studio и до этого не использовали оптимизационные диагностики – то это отличный случай, чтобы попробовать их в деле.

Популярные статьи по теме
MuditaOS: зазвонит ли ваш будильник? Часть 1

Дата: 01 Фев 2022

Автор: Владислав Столяров

Операционные системы – это тот вид софта, для которого качество исходного кода особенно критично. В этот раз под объективы анализатора PVS-Studio попала MuditaOS. Так давайте же посмотрим, что нам уд…
По заказам Embedded-разработчиков: ищем ошибки в Amazon FreeRTOS

Дата: 31 Окт 2019

Автор: Георгий Грибков

Каждый, кто программирует микроконтроллеры, наверняка знает о FreeRTOS, или по крайней мере слышал об этой операционной системе. Ребята из Amazon решили расширить возможности этой операционной систем…
Статический анализатор кода PVS-Studio 6.22 адаптирован для ARM-компиляторов (Keil, IAR)

Дата: 02 Мар 2018

Автор: Андрей Карпов

PVS-Studio - это статический анализатор кода для поиска ошибок и потенциальных уязвимостей в коде программ на языке C, C++ и C#. Мы давно радуем читателей нашего блога проверкой открытых проектов и р…

Комментарии (0)

Следующие комментарии
Этот сайт использует куки и другие технологии, чтобы предоставить вам более персонализированный опыт. Продолжая просмотр страниц нашего веб-сайта, вы принимаете условия использования этих файлов. Если вы не хотите, чтобы ваши данные обрабатывались, пожалуйста, покиньте данный сайт. Подробнее →
Принять