Comments 14
Давно читаю эти разборы, но только сейчас пришла в голову мысль попросить проверить kdenlive. Как у пользователя, есть большое подозрение, что материала вам там будет предостаточно :)
А почему вы описываете только пользу? А как же вред, чтоб ROI посчитать?
Помню только раз это предлагали, но стоит тогда в фак добавить что-ли =)
Насчёт "неправильной реализации блокировки с двойной проверкой" — здесь она вполне себе правильная до тех пор, пока в тех местах, в которых идёт дальнейшая работа с ui_storage (и которые здесь не показаны), до обращения к нему, как положено, захватывается соответствующий мутекс. Поскольку ui_storage_ensure() не возвращает указатель или ссылку на ui_storage для дальнейшего использования где-то ещё, то именно в этом конкретном коде нет риска, что будет обращение к внутренностям ui_storage до его полной инициализации из-за реордеринга, некогерентности кешей и прочего.
Хотя да, я не совсем прав — не обратил внимания, что сам указатель ui_storage не atomic, это проблема. Он должен быть atomic с доступом хотя бы memory_order_relaxed — дополнительные fences и ограничения на ордеринг тут по идее не нужны, мутекса достаточно, а вот операции с самим указателем все-таки должны быть по-хорошему атомарными.
дополнительные fences и ограничения на ордеринг тут по идее не нужны, мутекса достаточно
Проблема в том, что в конструкции
p = new P();происходят три действия:
- Выделение памяти
- Конструирование объекта по полученному адресу
- Запись адреса в переменную
И при определённых условиях компилятор может переставить запись в переменную до конструирования объекта. Как следствие первое чтение (не под мьютексом) считает адрес объекта, который ещё не сконструирован, и его использование приведёт к неопределённому поведению.
Теперь предположим, что мы сделаем так, как вы сказали
p.store(new P(), std::memory_order_relaxed);Может ли теперь компилятор переупорядочить действия?
Всё же здесь вызывается функция с результатом new-expression в качестве аргумента, но ведь и компилятор может в тело этой функцию «подсмотреть» и понять, что ей важно именно побитовое значение адреса, а вовсе не корректно сконструированный объект по этому адресу. Так что с этой точки зрения вроде как может переупорядочить.
С другой стороны это атомарная переменная, и компилятор понимает, что вполне законно её чтение в другом потоке, а вот там уже вполне могут использовать адрес для обращения к объекту (иначе зачем нам эта переменная?). С этой стороны получается, что переупорядочивание невозможно.
Но тут есть и второй источник переупорядочивания — процессор. И вот он вполне может при отсутствии должного барьера памяти сперва исполнить запись адреса в переменную, а уже потом — запись поля объекта из конструктора. Что опять таки приводит нас к потенциальному неопределённому поведению. Насколько я понимаю, такое переупорядочивание запись-запись невозможно на x86/x64, но вполне возможно на других архитектурах.
Так что придётся всё же чётко указать наши намерения
p.store(new P(), std::memory_order_release);Но тогда возникает вопрос: а читать с memory_order_relaxed можно?
Ну, когда мы читаем во второй раз под мьютексом — можно. Мьютекс обеспечит нам необходимые барьеры оптимизации и памяти, потому что изменяем переменную мы тоже под мьютексом. А в первый раз?
А вот в первый раз не можем, нам потребуется memory_order_acquire. Дело в том, что при сохранении переменной процессор всё ещё может переупорядочить записи указателя и полей в объекте. Просто потом он исполнит инструкцию, синхронизирующую видимое состояние памяти для разных ядер/процессоров (обеспечит когерентность кэша). И вот если другой поток прочитает значение указателя до исполнения этой инструкции, то вполне может «увидеть» его значение, но не значение полей объекта — и опять наше любимое неопределённое поведение. В модели памяти C++ это выражается отношением «синхронизируется-с» (synchronizes-with). И такое отношение есть между memory_order_acquire и memory_order_release, но memory_order_relaxed не синхронизируется ни с чем.
Так что код из статьи надо переделать вот так
typedef struct bNodeTree {
....
std::atomic<NodeTreeUIStorage*> ui_storage;
} bNodeTree;
static void ui_storage_ensure(bNodeTree &ntree)
{
/* As an optimization, only acquire a lock if the UI storage doesn't exist,
* because it only needs to be allocated once for every node tree. */
if (ntree.ui_storage.load(std::memory_order_aquire) == nullptr) {
std::lock_guard<std::mutex> lock(global_ui_storage_mutex);
/* Check again-- another thread may have allocated the storage
while this one waited. */
if (ntree.ui_storage.load(std::memory_order_relaxed) == nullptr) {
ntree.ui_storage.store(new NodeTreeUIStorage(), std::memory_order_release);
}
}
}На данную тему есть интересная статья — Double-Checked Locking is Fixed In C++11. Да и вообще в этом блоге много чего интересного на тему многопоточности и атомарных переменных.
Дело в том, что при сохранении переменной процессор всё ещё может переупорядочить записи указателя и полей в объекте. Просто потом он исполнит инструкцию, синхронизирующую видимое состояние памяти для разных ядер/процессоров (обеспечит когерентность кэша)
Тут я, конечно, соврал. Пишущий поток не переупорядочит запись и выполнит синхронизацию перед записью атомарной переменной. Дело в другом — читающий поток должен выполнить инструкцию синхронизации, чтобы увидеть не только значение указателя, но и остальных данных. Для memory_order_relaxed это необязательно ввиду отсутствия соответствующего отношения «синхронизируется-с».
Как следствие первое чтение (не под мьютексом)
Какое «первое чтение (не под мьютексом)»? Там нет никакого чтения. Покажите мне в обсуждаемом коде чтение содержимого по адресу, на который указывает ui_storage, не под мьютексом:
static void ui_storage_ensure(bNodeTree &ntree)
{
/* As an optimization, only acquire a lock if the UI storage doesn't exist,
* because it only needs to be allocated once for every node tree. */
if (ntree.ui_storage == nullptr) {
std::lock_guard<std::mutex> lock(global_ui_storage_mutex);
/* Check again-- another thread may have allocated the storage
while this one waited. */
if (ntree.ui_storage == nullptr) {
ntree.ui_storage = new NodeTreeUIStorage();
}
}
}
Этого чтения там нет. В каком бы порядке ни выполнялись те действия, о которых вы пишете, ничего плохого в данной конкретной функции произойти не может (ну если не считать неатомарности самого указателя). В других местах — там да, будет нужен мутекс, о чем я уже написал. Но это в других местах, которых тут нет.
Какое «первое чтение (не под мьютексом)»? Там нет никакого чтения. Покажите мне в обсуждаемом коде чтение содержимого по адресу, на который указывает ui_storage, не под мьютексом
Ну, так вот же:
if (ntree.ui_storage == nullptr)В каком бы порядке ни выполнялись те действия, о которых вы пишете, ничего плохого в данной конкретной функции произойти не может
Ну, как скажете. Ваш код — вам и отлаживать. Но я вас предупредил.
Прочитал статью про блокировку с двойной проверкой. Для меня это был взрыв мозга. Огромное спасибо автору!
Как вы вообще оцениваете качество кода Blender? Вы назвали несколько найденных ошибок из свежих изменений, и мне любопытно, насколько их в принципе много было во всем коде проекта, какие наиболее частые?
Сложно сказать. Я вижу поток ошибок и недочётов, но мне просто неинтересно на них как-то реагировать. А ждал, когда будет одновременно парочка интересных багов, про которые можно сделать заметку. Например, сейчас ткнул мышкой в один из недавних прогонов от 25 февраля 2021. Вижу 3 предупреждения. Два мне с ходу непонятны, с ними требуется разбираться, а мне это скучно (лень). Для простоты будем считать их ложными срабатываниями. Остаётся:
Предупреждение PVS-Studio: V617: Consider inspecting the condition. The 'T_OVERRIDE_CENTER' argument of the '|' bitwise operation contains a non-zero value. transform_convert.c. 85
Это явная ошибка. Из-за того что перепутали оператор, условие будет всегда истинным. Правильный вариант:
Но ранее я посмотрел на эту ошибку и закрыл отчёт. Не буду же я с каждой такой ошибкой бегать и всем её показывать :). Мне будет скучновато всё подобное описывать, а подписчикам – читать.
Сообщил я разработчикам проекта? Нет. Это их работа – обеспечивать качество кода. Я не могу помочь всем. Вокруг меня тысячи открытых проектов, в которых каждый день появляются новые баги. Даже если я захочу, много я не направлю. Намного продуктивнее популяризировать методологию статического анализа кода. Тогда такие ошибки сразу будут находить сами разработчики проектов.
Если я постоянно вижу всё новые и новые ошибки в Blender, почему я не говорю, что качество кода низкое? Ответ: проект большой. А чем больше проект, тем больше ошибок появляется в коде. Так что я не берусь судить в данном случае. Впрочем, статический анализ этому проекту явно не помешает.
P.S. Я немного побуду злодеем. Я не буду агитировать разработчиков Blender начать использовать PVS-Studio. А то я лишусь возможности находить ошибки в этом интересном проекте, и мне придётся искать другой проект :).
typedef enum {
....
T_OVERRIDE_CENTER = 1 << 18,
....
} eTFlag;
....
if (t->flag | T_OVERRIDE_CENTER) {Предупреждение PVS-Studio: V617: Consider inspecting the condition. The 'T_OVERRIDE_CENTER' argument of the '|' bitwise operation contains a non-zero value. transform_convert.c. 85
Это явная ошибка. Из-за того что перепутали оператор, условие будет всегда истинным. Правильный вариант:
if (t->flag & T_OVERRIDE_CENTER) {Но ранее я посмотрел на эту ошибку и закрыл отчёт. Не буду же я с каждой такой ошибкой бегать и всем её показывать :). Мне будет скучновато всё подобное описывать, а подписчикам – читать.
Сообщил я разработчикам проекта? Нет. Это их работа – обеспечивать качество кода. Я не могу помочь всем. Вокруг меня тысячи открытых проектов, в которых каждый день появляются новые баги. Даже если я захочу, много я не направлю. Намного продуктивнее популяризировать методологию статического анализа кода. Тогда такие ошибки сразу будут находить сами разработчики проектов.
Если я постоянно вижу всё новые и новые ошибки в Blender, почему я не говорю, что качество кода низкое? Ответ: проект большой. А чем больше проект, тем больше ошибок появляется в коде. Так что я не берусь судить в данном случае. Впрочем, статический анализ этому проекту явно не помешает.
P.S. Я немного побуду злодеем. Я не буду агитировать разработчиков Blender начать использовать PVS-Studio. А то я лишусь возможности находить ошибки в этом интересном проекте, и мне придётся искать другой проект :).
> и мне любопытно, насколько их в принципе много было во всем коде проекта, какие наиболее частые?
Много. Но я их не изучал. Кстати, мы и раньше много находили: www.viva64.com/ru/b/0145, www.viva64.com/ru/b/0413
Много. Но я их не изучал. Кстати, мы и раньше много находили: www.viva64.com/ru/b/0145, www.viva64.com/ru/b/0413
Примечание 2. Читатели обратили внимание, что описание проблемы блокировки с двойной проверкой устарело. В C++17 язык гарантированно выполняет все побочные эффекты, связанные с выражением new T перед выполнением эффектов самого присваивания (operator =). Другими словами, начиная с C++17, эту часть можно назвать «уже пофиксили, это не баг». Однако, запись всё равно не является атомарной, и возможно возникновение состояния гонки. Чтобы этого избежать, указатель должен быть объявлен как атомарный: std::atomic<NodeTreeUIStorage *> ui_storage
Поскольку я читал статью раньше, то вчера во время написания комментария не заметил это примечание.
Дело в том, что эти изменения в C++17 для случая, когда мы можем заметить разницу в состоянии абстрактной машины C++ в зависимости от последовательности вычислений. Например, код
i = ++i + 1;приводил к неопределённому поведению до C++17, поскольку вычисления инкремента i и присваивания не упорядочивались относительно друг друга. Но если взять код
p = new A();то заметим ли мы разницу в состоянии абстрактной машины, если сначала произойдёт присвоение указателя, а уже потом — инициализация объекта?
Да, мы можем заметить разницу, если 1) конструктор выбросит исключение или 2) в конструкторе будут вычисления на основе текущего значения p.
Но если ни одно из этих условий не выполнено, то различий не будет. Или же компилятор встроит вызов конструктора так, что выброс исключения и/или вычисления на основе текущего p будут перед присваиванием p, а все остальные вычисления после, то разницы мы так же не заметим.
Ну, а пока мы не видим различий в поведении программы, то руки компилятора в плане оптимизации развязаны, и он всё ещё может осуществить присваивание указателя до инициализации полей. И не только может, но и делает.
Но даже если компилятор сгенерирует команды в ожидаемом нами порядке, и мы даже сделаем указатель атомарным с доступом memory_order_relaxed, этого недостаточно. Нам всё же необходимо, чтобы другие потоки увидели значение указателя не раньше, чем правильно инициализированные поля объекта, на который он указывает. А для этого необходимы барьеры памяти aquire — release.
Так что изменения в C++17 ничего в данном вопросе не исправили. Они просто уменьшили количество ситуаций, в которых может произойти неопределённое поведение, но проблема в данном случае не в нём.
Sign up to leave a comment.
PVS-Studio, Blender: цикл заметок о пользе регулярного использования статического анализа