PVS-Studio впечатлен качеством кода Abbyy NeoML

02 Июл 2020

Автор: Виктория Ханиева

Запуск анализа
Результаты анализа
Заключение

На днях компания ABBYY опубликовала исходный код своего фреймворка NeoML. Нам предложили проверить эту библиотеку с помощью PVS-Studio. Это интересный проект с точки зрения анализа, так что мы не стали откладывать его в долгий ящик. Чтение этой статьи не займет у вас много времени, так как проект оказался высокого качества :).

Исходный код NeoML можно найти на GitHub. Этот фреймворк является кроссплатформенным и предназначен для реализации моделей машинного обучения. Он используется разработчиками компании ABBYY для решения задач компьютерного зрения, обработки естественного языка, в том числе обработки изображений, анализа документов и так далее. В настоящее время поддерживаются такие языки программирования как C++, Java, Objective-C, и скоро к этому списку должен добавиться Python. Основной язык, на котором был написан сам фреймворк, это С++.

Запуск анализа

Запустить анализ на этом фреймворке было очень просто. После генерации проекта Visual Studio в CMake я запустила анализ PVS-Studio в Visual Studio на интересующие нас проекты из Solution, исключая сторонние библиотеки. Помимо самого NeoML в решении присутствовали еще такие библиотеки от ABBYY как NeoOnnx и NeoMathEngine. Их я также включила в список проектов, для которых запускался анализ.

Результаты анализа

Конечно же очень хотелось найти какие-нибудь страшные ошибки, но... код оказался достаточно чистым и предупреждений было получено всего-ничего. Вполне вероятно, что при разработке уже использовался статический анализ. Многие из предупреждений были срабатываниями одних и тех же диагностик на схожие участки кода.

Например, в этом проекте очень часто происходит вызов виртуального метода из конструктора. В целом это опасный подход. На такие случаи реагирует диагностика V1053: Calling the 'foo' virtual function in the constructor/destructor may lead to unexpected result at runtime. В общей сложности было выдано 10 таких предупреждений. Подробнее о том, почему это опасная практика и какие проблемы она вызывает можно почитать в этой статье Скотта Майерса "Never Call Virtual Functions during Construction or Destruction". Однако, по всей видимости разработчики понимают, что они делают, и ошибок здесь нет.

Также есть 11 предупреждений среднего уровня диагностики V803 из раздела "микрооптимизаций". Эта диагностика рекомендует заменить постфиксный инкремент на префиксный, если предыдущее значение итератора не используется. В случае постфиксного инкремента создается лишний временный объект. Конечно же это не является ошибкой, просто маленькая деталь. Если такая диагностика неинтересна, то при использовании анализатора можно её попросту выключить. Ну и в принципе, набор "микро-оптимизаций" и выключен по умолчанию.

Собственно, думаю вы понимаете, что раз мы дошли до разбора в статье таких мелочей, как инкремент итератора, то значит вообще всё хорошо и мы просто не знаем к чему попридираться.

Очень часто некоторые диагностики могут быть неприменимы или неинтересны для пользователя и лучше не есть кактус, а потратить немного времени на настройку анализатора. Подробнее о шагах, которые стоит предпринять для того, чтобы сразу подобраться поближе к наиболее интересным срабатываниям анализатора, можно почитать в нашей статье "Как быстро посмотреть интересные предупреждения, которые выдает анализатор PVS-Studio для C и C++ кода?"

Среди срабатываний из раздела "микрооптимизаций" также есть интересные предупреждения диагностики V802, которая рекомендует расположить поля структуры по убыванию размеров типов, что позволяет снизить размер структуры.

V802 On 64-bit platform, structure size can be reduced from 24 to 16 bytes by rearranging the fields according to their sizes in decreasing order. HierarchicalClustering.h 31

struct CParam {
  TDistanceFunc DistanceType; 
  double MaxClustersDistance;
  int MinClustersCount; 
};

Всего лишь поменяв местами поле MaxClustersDistance с типом double и перечислитель DistanceType можно снизить размер структуры с 24 до 16 байт.


struct CParam {
  TDistanceFunc DistanceType; 
  int MinClustersCount; 
  double MaxClustersDistance; 
};

TDistanceFunc это enum, так что его размер эквивалентен int или меньшему типу, поэтому его стоит перенести в конец структуры.

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

В целом весь код написан аккуратно и разборчиво, но диагностика V807 указала на пару мест, которые можно сделать чуть более оптимальными и читабельными. Приведу наиболее наглядный пример:

V807 Decreased performance. Consider creating a reference to avoid using the same expression repeatedly. GradientBoostFullTreeBuilder.cpp 469

Обращение к curLevelStatistics[i]->ThreadStatistics[j] можно заменить на обращение к отдельной переменной. В этой цепочке нет вызова каких-то сложных методов, так что особой оптимизации здесь может и не будет, но код, на мой взгляд, будет читаться куда проще и выглядеть компактнее. К тому же, при поддержке этого кода в дальнейшем будет явно видно, что необходимо обращение именно по этим индексам и ошибки здесь нет. Для наглядности приведу код с заменой на переменную:

auto threadStatistics = curLevelStatistics[i]->ThreadStatistics[j];

if(threadStatistics.FeatureIndex != NotFound ) {
  if(   threadStatistics.Criterion > criterion
     || ( .... ))
  {
    criterion = threadStatistics.Criterion;
    curLevelStatistics[i]->FeatureIndex    = threadStatistics.FeatureIndex;
    curLevelStatistics[i]->Threshold       = threadStatistics.Threshold;
    curLevelStatistics[i]->LeftStatistics  = threadStatistics.LeftStatistics;
    curLevelStatistics[i]->RightStatistics = threadStatistics.RightStatistics;
  }
}

Заключение

Как видите, с точки зрения статического анализа, кодовая база этого фреймворка оказалось очень чистой.

Стоит понимать, что один запуск анализа на активно разрабатываемый проект слабо отражает необходимость в статическом анализе, так как многие ошибки, особенно если они были критическими, уже были обнаружены и другими путями, но куда более затратными по времени и ресурсам. Чуть подробнее этот момент был разобран в статье "Ошибки, которые не находит статический анализ кода, потому что он не используется".

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

#Cpp