Re[13]: Тестовое задание ...
От: Evgeny.Panasyuk Россия  
Дата: 16.06.15 14:11
Оценка: 2 (1) +1
Здравствуйте, Selavi, Вы писали:

S>>>Заметьте, что моей реализации, которую все обос*али вышеописанных проблем нет)

EP>>Там есть проблемы покруче, например:
EP>>* одно и то же задание может выполнится несколько раз, так как не учитываются spurious wakeup.
EP>>* выполнение не заданного задания (segfault).
EP>>* отсутствие join для CThreadPoolX, в результате std::terminate.
S>Да, Вы правы...вот черт( Наконец то хороший анализ!

А до этого что, плохие были? Люди потратили своё время, посмотрели код, даже если тебе обидно — постарайся сдерживать эмоции.

S>А не могли бы подробней про 2 и 3?


2. Рассмотри ситуацию когда рабочий поток создался, не получил ни одного задания, а потом разрушился. В этом случае поток проснётся и попробует выполнять this->task, который указывает в никуда.

3. Управляющий поток CThreadPoolX создаётся, но нигде ни join'ится, ни detach'ится. В таком случае деструктор std::thread сделает std::terminate.

Наверняка есть ещё подобные проблемы, но дальше не смотрел.
Re[14]: Тестовое задание ...
От: Evgeny.Panasyuk Россия  
Дата: 16.06.15 14:33
Оценка: 1 (1)
Здравствуйте, GreenTea, Вы писали:

GT>>>Величина истории ограничена 100, если надо можно вынести это в параметр пула.

EP>>Требуется обойти всех клиентов по порядку + зацикливание на начало, всё
EP>>Зачем так сложно-то? В getNextClientTask создаётся два отображения, крутятся три цикла, всё это под мьютексом, да ещё и введён "параметр аппроксимации решения"
GT>Не понял. Какое еще зацикливание? Возможно сложно, но это первое что пришло в голову.

Есть три клиента. Их задания нужно обрабатывать циклически — 1, 2, 3, 1, 2, 3, 1, 2, 3, 1, 2, 3 ... — с поправкой на то, что задания одного клиента параллельно не выполняются.
Решение в лоб — просто создать список всех клиентов + общий циклический итератор.

GT>Хотелось бы посмотреть на ваше решение.


Например так:
* Для каждого клиента создаётся отдельная очередь заданий.
* Создаётся очередь из очередей всех клиентов.
* Требуется биективное отображение "клиент -> очередь". На add_task, если такой клиент уже был — то находим его очередь через отображение, и добавляем в неё, если же нет — добавляем клиента в отображение, ставим задание в очередь, а саму очередь добавляем (push) в конец очереди очередей клиентов. В случае если нужно добавить пачку заданий от одного клиента — то потребуется меньше синхронизации, так как не нужно каждый раз искать его очередь в отображении.
* Каждый рабочий поток делает в цикле try_pop из очереди очередей клиентов. Если получилось достать очередь клиента — то делаем try_pop задания из его очереди, если задание есть — то выполняем его, потом очередь клиента ставится (push) в конец общей очереди.
* Если потребуется, пустые (exhausted) очереди клиентов можно удалять, но тогда скорей всего lock free не получится, по крайней мере просто.
Re[15]: Тестовое задание ...
От: GreenTea  
Дата: 16.06.15 15:26
Оценка:
Здравствуйте, Evgeny.Panasyuk, Вы писали:

Да, так будет проще.
Re[3]: Тестовое задание ...
От: Kernan Ниоткуда https://rsdn.ru/forum/flame.politics/
Дата: 16.06.15 17:17
Оценка:
Здравствуйте, Evgeny.Panasyuk, Вы писали:

EP>Здравствуйте, GreenTea, Вы писали:


GT>>Если кто шарит в java, прошу поревьювить. Я сам с ручной многопоточностью редко работал, так что могут быть косяки..


EP>Представь ситуацию, когда в первой половине tasks у нас задачи от первого клиента, а во второй — от второго, и всего один рабочий поток. Он будет выполнять все задания по списку, то есть сначала все от первого клиента, хотя по заданию должен чередовать.

Почему бы не сделать roud-robin с ид клиентами и бегать по ней выбора очередного клиента и уже из клиентской очереди выбирать таск на выполнение? Тогда ни у одного клиента не будет долгого простоя.
Sic luceat lux!
Re[14]: Тестовое задание ...
От: Selavi  
Дата: 16.06.15 18:01
Оценка:
Здравствуйте, Evgeny.Panasyuk, Вы писали:

EP>А до этого что, плохие были?


Да, были плохие. В основном они касались оформления кода и внимательности при чтении задания. Проблем реализации многопоточного пула коснулись только Вы.

EP>Люди потратили своё время, посмотрели код, даже если тебе обидно — постарайся сдерживать эмоции.


Спасибо за совет, но лучше давайте говорить о программировании.

S>>А не могли бы подробней про 2 и 3?


EP>2. Рассмотри ситуацию когда рабочий поток создался, не получил ни одного задания, а потом разрушился. В этом случае поток проснётся и попробует выполнять this->task, который указывает в никуда.


Да, но это дело обернуто в try..catch и ничего плохого не случится. Ошибка залогируется и едем дальше. Хотя, конечно, надо добавить проверку на nullptr

EP>3. Управляющий поток CThreadPoolX создаётся, но нигде ни join'ится, ни detach'ится. В таком случае деструктор std::thread сделает std::terminate.


Как так? В деструкторе же join-ится

CThreadPoolX::~CThreadPoolX()
{
    terminated = true;
    jobCV.notify_one();
    thread.join();
}
Re[4]: Тестовое задание ...
От: Evgeny.Panasyuk Россия  
Дата: 16.06.15 18:32
Оценка:
Здравствуйте, Kernan, Вы писали:

GT>>>Если кто шарит в java, прошу поревьювить. Я сам с ручной многопоточностью редко работал, так что могут быть косяки..

EP>>Представь ситуацию, когда в первой половине tasks у нас задачи от первого клиента, а во второй — от второго, и всего один рабочий поток. Он будет выполнять все задания по списку, то есть сначала все от первого клиента, хотя по заданию должен чередовать.
K>Почему бы не сделать roud-robin с ид клиентами и бегать по ней выбора очередного клиента и уже из клиентской очереди выбирать таск на выполнение? Тогда ни у одного клиента не будет долгого простоя.

Да, думаю это самое простое, я уже выше предлагал:

EP>* Каждый рабочий поток делает в цикле try_pop из очереди очередей клиентов. Если получилось достать очередь клиента — то делаем try_pop задания из его очереди, если задание есть — то выполняем его, потом очередь клиента ставится (push) в конец общей очереди.

Re[15]: Тестовое задание ...
От: Evgeny.Panasyuk Россия  
Дата: 16.06.15 19:05
Оценка:
Здравствуйте, Selavi, Вы писали:

EP>>А до этого что, плохие были?

S>Да, были плохие. В основном они касались оформления кода и внимательности при чтении задания. Проблем реализации многопоточного пула коснулись только Вы.

Проблемы это же не только явные баги, но и неудачные технические решения. Например выше говорили про лишний поток, или проблемы с жизненным циклом объектов — это вполне объективные недостатки, а не "просто оформление". А не следование API заданному в ТЗ — вообще выглядит как намеренная диверсия.

S>>>А не могли бы подробней про 2 и 3?

EP>>2. Рассмотри ситуацию когда рабочий поток создался, не получил ни одного задания, а потом разрушился. В этом случае поток проснётся и попробует выполнять this->task, который указывает в никуда.
S>Да, но это дело обернуто в try..catch и ничего плохого не случится. Ошибка залогируется и едем дальше.

Это Undefined Behaviour — в лучшем случае получишь segmentation fault, в худшем — выполнение произвольного кода с последующим penetration.

S>Хотя, конечно, надо добавить проверку на nullptr


Это не поможет, там остаётся возможность выполнения последнего задания дважды (даже не считая spurious wakeup).

EP>>3. Управляющий поток CThreadPoolX создаётся, но нигде ни join'ится, ни detach'ится. В таком случае деструктор std::thread сделает std::terminate.

S>Как так? В деструкторе же join-ится
S>
S>CThreadPoolX::~CThreadPoolX()
S>{
S>    terminated = true;
S>    jobCV.notify_one();
S>    thread.join();
S>}
S>


Только что скачал этот файл заново:
CThreadPoolX::~CThreadPoolX()
{
}
Re[16]: Тестовое задание ...
От: Selavi  
Дата: 16.06.15 19:26
Оценка:
Здравствуйте, Evgeny.Panasyuk, Вы писали:

EP>Проблемы это же не только явные баги, но и неудачные технические решения. Например выше говорили про лишний поток, или проблемы с жизненным циклом объектов — это вполне объективные недостатки, а не "просто оформление". А не следование API заданному в ТЗ — вообще выглядит как намеренная диверсия.


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

EP>Это не поможет, там остаётся возможность выполнения последнего задания дважды (даже не считая spurious wakeup).


Вот последняя версия рабочего метода потока. Разве тут есть вышеописанные проблемы?

void CWorkThread::ThreadFunc()
{
    std::unique_lock<std::mutex> lock(mutex);

    while (!this->terminated)
    {
        while (!this->task)
          cv.wait(lock);

        std::lock_guard<std::mutex> lockWorkFlag(busyFlag);

        try
        {
            std::cout << "Started  Client " + std::to_string(clientID)+" in workerThread " + std::to_string((int)this) + "\r\n";
            if (this->task)
                this->task->Execute();
        }
        catch (const std::exception &e)
        {
            //Log
        }

        this->task = nullptr;

        std::cout << "Finished Client " + std::to_string(clientID) + " in workerThread " + std::to_string((int)this) + "\r\n";

        clientTaskFinisher->FinishClientTask(clientID);
    }
}



EP>Только что скачал этот файл заново:


Да, мой косяк) Я добавил код в деструктор уже после того, как выложил файл.
Re[15]: Тестовое задание ...
От: Олег К.  
Дата: 16.06.15 19:34
Оценка:
ГВ>Не принимай близко к сердцу, прямо к тебе это не относится и на самом деле отклонения от ТЗ не сопровождаются развешиванием таких ярлыков. Меня просто Олег немало рассмешил, вот я и расслабился. Блин, ну надо же такое ляпнуть
Автор: Олег К.
Дата: 15.06.15
: "Где в задании было написано, что отклоняться от интерфейса нельзя?" Гомгардия!


Где в задании написано, что нужно создавать объект этого класса? Вот тебе другой пример. Сказано что написать только один класс и все.

Ну и ты не понимаешь самой сути задания. Задание было решить такую-то задачу. А все эти интерфейсы — это второстепенное, и нужно быть совсем уж идиотом чтобы считать, что в реальной задаче ТС сделал бы по-своему. Ну и интерфейсы по работе зачастаю обсуждаются между коллегами все-таки и очень часто могут меняться в силу тех или иных обстоятельств.
Re[10]: Тестовое задание ...
От: Олег К.  
Дата: 16.06.15 19:36
Оценка:
ОК>>Почему эта ситуация не была описана в задании? Далее, почему тут можно кандидату выбирать по вкусу а изменить интерфейс как ему нравится нельзя?

S>Собственно, прямо в точку. В соседней ветке я говорил кому то, что ТЗ было получено через кадровое и мне не хотелось устраивать испорченный телефон по уточнению задания.

S>Кстати, если бы ТЗ мне дали бы на собеседовании, то я бы, однозначно, задал уточняющие вопросы по нему и скорее всего этого топика бы не было.

До них этого не доходит. Сложно им такое понять.
Re[17]: Тестовое задание ...
От: Evgeny.Panasyuk Россия  
Дата: 16.06.15 19:46
Оценка: 2 (1)
Здравствуйте, Selavi, Вы писали:

EP>>Это не поможет, там остаётся возможность выполнения последнего задания дважды (даже не считая spurious wakeup).

S>Вот последняя версия рабочего метода потока. Разве тут есть вышеописанные проблемы?

this->task должен инициализироваться в конструкторе на nullptr (этого не было).
RunTask устанавливает this->task не под мьютексом — там, насколько я вижу, возможен data race при spurious wakeup:
void CWorkThread::RunTask
    this->task = task; // #1

void CWorkThread::ThreadFunc
        while (!this->task) // #2
              cv.wait(lock);


Далее, this->terminated — это обычный bool, соответственно тут тоже data race.
Re[10]: Тестовое задание ...
От: Олег К.  
Дата: 16.06.15 19:47
Оценка:
ОК>>>>Пулу при инициализации задается число рабочих потоков:
ОК>>>>bool CThreadPool::Init(int _numThreads);
ОК>>>>Почему количество потоков не задается в конструкторе?
ГВ>>>Стереотипичный ответ: потому что предполагается двухфазная инициализация.
ОК>>Ну и нафига? Уж лучше было бы эту функцию назвать что-нибудь вроде CThreadPool::StartThreads() чтобы изменить семантику. Потоки ведь нао будет еще как-то останавливать, уничтожать, ждать их завершения и т.д.

ГВ>А какая тебе разница? Задание поставлено именно так, и никак иначе. Естественным следствием из наличия Init будет появление и Shutdown.


Мне не нравится такой интерфейс. У Майкрософта тоже не замечал таких Инитов() и Шатдаунов(). (интересно, есть ли в кью-ти такое?)

ОК>>>>Что должна вернуть функция?

ГВ>>>Внезапно: true, если все в порядке и false, если не удалось создать все запланированные потоки.
ОК>>Это твои домыслы (с коими я сам также согласен), но дай я поутрирую чуточку. Это ж сколько тебе нужно создать потоков, чтобы функция вернула ложь?

ГВ>Это может быть и второй по счёту поток, если система перегружена. Всё, что тут надо сделать — это отследить ошибку создания потока и прекратить инициализацию, уничтожив уже созданные потоки.


А почему не graceful degradation? Пытаешься выделить столько ресурсов сколько можно и работаешь уже с ними. Почему этого нет в задании? Ты ответь-то на вопрос.

ОК>>>>Что делать если нельзя создать нужное количество потоков? Убивать уже созданные или оставить их так как есть? Где описание всего этого?

ГВ>>>Ответы простейшие: а) вернуть false, б) убрать созданные.
ОК>>Смотри выше. Ну и где описание оного?

ГВ>Это стереотип, которого, ИМХО, вполне достаточно для тестового задания.


Ты о чем это? Код требует точностей. Поэтому и описание должно быть точным.

ОК>>>>Почему от ТС требуются юнит-тесты а составители не желают четко описать чего хотят?

ГВ>>>Потому что не нужно быть семи пядей во лбу, чтобы ответить на поставленные вопросы.
ОК>>То есть получается, что кто-то желает многого но сам хочет сделать малого (составить нормальное задание)?

ГВ>Да нормальное оно вполне.


Я так не считаю.

ОК>>>>В каком состоянии должны быть потоки? Running или suspended? Где описание?

ГВ>>>Это можно спросить у тех, кто дал задание. Но подразумевается, что кандидат сам выберет то, что ему по вкусу.
ОК>>Почему эта ситуация не была описана в задании? Далее, почему тут можно кандидату выбирать по вкусу а изменить интерфейс как ему нравится нельзя?

ГВ>Потому что интерфейс — это та часть, с которой обычно работают другие и согласуют его отдельно от деталей реализации. Здесь как раз промоделирована ситуация, когда интерфейс предписан извне, а реализация оставлена на усмотрение разработчика.


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

ОК>>>>Что делает подчеркивание перед параметром? Обычно такая хрень — спереди или сзади — вводится для членов. Какая у них венгерская нотация для самих локальных переменных и членов?

ГВ>>>Стиль внешнего оформления задан, внутри пиши как хочешь или уточни у тех, кто дал задание.
ОК>>Какой-то хреновый стиль но пусть будет по-твоему. Почему не описан внутренний стиль?

ГВ>См. выше.


Это тебе надо смотреть выше. Ты не понимаешь самой сути задания. Тебе не понравился его код (ок, я согласен с тобой в этом) и тебе нужно найти аргументы чтобы обосновать свою точку зрения. Вот ты и прицепился к мелочам.
Re[18]: Тестовое задание ...
От: Selavi  
Дата: 16.06.15 20:45
Оценка:
Здравствуйте, Evgeny.Panasyuk, Вы писали:

EP>RunTask устанавливает this->task не под мьютексом — там, насколько я вижу, возможен data race при spurious wakeup:


Не должно быть data race, поскольку в вызывающем коде перед RunTask выполняется проверка IsFree, которая в свою очередь, проверяет доступность мютекса, которым владеет рабочий поток в момент выполнения задания (пусть даже в результате spurious wakeup)
Хотя...если поток проснется между проверкой и вызовом RunTask, то будет печаль..

Видимо лучше будет добавить в RunTask захват мютекса lockWorkFlag?


EP>Далее, this->terminated — это обычный bool, соответственно тут тоже data race.


Вот это я не понял. Как тут возможна гонка?


з.ы.

Если Вам удобней, то все лежит на github — https://github.com/uuFinally/MultiThreadingTest
Re[19]: Тестовое задание ...
От: Evgeny.Panasyuk Россия  
Дата: 16.06.15 21:08
Оценка: 2 (1)
Здравствуйте, Selavi, Вы писали:

EP>>RunTask устанавливает this->task не под мьютексом — там, насколько я вижу, возможен data race при spurious wakeup:

S>Не должно быть data race, поскольку в вызывающем коде перед RunTask выполняется проверка IsFree, которая в свою очередь, проверяет доступность мютекса, которым владеет рабочий поток в момент выполнения задания (пусть даже в результате spurious wakeup)
S>Хотя...если поток проснется между проверкой и вызовом RunTask, то будет печаль..

Об этом и речь.

S>Видимо лучше будет добавить в RunTask захват мютекса lockWorkFlag?


Это уберёт data race. Только надо делать правильный захват — на github'е сейчас неправильный.

EP>>Далее, this->terminated — это обычный bool, соответственно тут тоже data race.

S>Вот это я не понял. Как тут возможна гонка?

Чтения и записи в bool terminated никак не упорядоченны между потоками. Вот что говорит стандарт по этому поводу:

Two expression evaluations conflict if one of them modifies a memory location (1.7) and the other one accesses or modifies the same memory location.
...
The execution of a program contains a data race if it contains two conflicting actions in different threads, at least one of which is not atomic, and neither happens before the other. Any such data race results in undefined behavior.

Re[20]: Тестовое задание ...
От: Selavi  
Дата: 16.06.15 21:31
Оценка:
Здравствуйте, Evgeny.Panasyuk, Вы писали:

EP>Это уберёт data race. Только надо делать правильный захват — на github'е сейчас неправильный.


эмм...а что в нем неправильного? Захват как захват

EP>Чтения и записи в bool terminated никак не упорядоченны между потоками. Вот что говорит стандарт по этому поводу:


Как же с этим бороться? Вешать еще один мютекс в деструктор?
Re[21]: Тестовое задание ...
От: Evgeny.Panasyuk Россия  
Дата: 16.06.15 22:06
Оценка:
Здравствуйте, Selavi, Вы писали:

EP>>Это уберёт data race. Только надо делать правильный захват — на github'е сейчас неправильный.

S>эмм...а что в нем неправильного? Захват как захват

Вот тут рабочий поток под мьютексом this->mutex:
    while (!this->task) // <--- under this->mutex
        cv.wait(lock);
А вот тут управляющий поток под мьютексом this->busyFlag:
    std::lock_guard<std::mutex> lockWorkFlag(busyFlag);
    this->task = task; // <--- under this->busyFlag


EP>>Чтения и записи в bool terminated никак не упорядоченны между потоками. Вот что говорит стандарт по этому поводу:

S>Как же с этим бороться? Вешать еще один мютекс в деструктор?

Мьютекс необязательно, достаточно атомарной переменной.
Re[22]: Тестовое задание ...
От: Selavi  
Дата: 16.06.15 22:28
Оценка:
Здравствуйте, Evgeny.Panasyuk, Вы писали:

EP>Вот тут рабочий поток под мьютексом this->mutex:

EP>
EP>    while (!this->task) // <--- under this->mutex
EP>        cv.wait(lock);
EP>
А вот тут управляющий поток под мьютексом this->busyFlag:

EP>
EP>    std::lock_guard<std::mutex> lockWorkFlag(busyFlag);
    this->>task = task; // <--- under this->busyFlag
EP>



Хм...получается, что мютекс busyFlag вообще не нужен и можно пользоваться только mutex для всех случаев.

Кстати, не удалось сходу нагуглить...что будет если cv.wait вызовется в момент, когда mutex не будет locked?
Re[23]: Тестовое задание ...
От: Evgeny.Panasyuk Россия  
Дата: 16.06.15 22:43
Оценка:
Здравствуйте, Selavi, Вы писали:

S>Кстати, не удалось сходу нагуглить...что будет если cv.wait вызовется в момент, когда mutex не будет locked?


Precondition violation
Re[5]: Тестовое задание ...
От: Tilir Россия http://tilir.livejournal.com
Дата: 17.06.15 05:50
Оценка:
Здравствуйте, Геннадий Васильев, Вы писали:

ГВ>- Нарушен стиль оформления имён параметров (clientID вместо _clientId).


Вообще-то этот пункт соискателю в плюс. По стандарту вы не имеете права использовать в прикладном коде имена, начинающиеся с underscore -- они все зарезервированы для нужд стандартной библиотеки.
Re[6]: Тестовое задание ...
От: Геннадий Васильев Россия http://www.livejournal.com/users/gesha_x
Дата: 17.06.15 07:29
Оценка:
Здравствуйте, Tilir, Вы писали:

ГВ>>- Нарушен стиль оформления имён параметров (clientID вместо _clientId).


T>Вообще-то этот пункт соискателю в плюс. По стандарту вы не имеете права использовать в прикладном коде имена, начинающиеся с underscore -- они все зарезервированы для нужд стандартной библиотеки.


В глобальном пространстве имён. Относительно локальных, насколько я помню, ничего не сказано.

В принципе, само по себе это "отклонение" не значит почти ничего, если бы не вот такие конструкции:

void CWorkThread::RunTask(int clientID, CTask* task)
{
    this->task = task;
    this->clientID = clientID;

    cv.notify_one();
}


Мне показалось, что если бы формальные параметры были оформлены иначе, чем имена членов класса, код был бы чуть проще.

Ну и потом, это я "обострил и преувеличил" ради того, чтобы ответить Славе, а то пришлось бы уходить в совсем уж отвлечённые материи.
Я знаю только две бесконечные вещи — Вселенную и человеческую глупость, и я не совсем уверен насчёт Вселенной. (c) А. Эйнштейн
P.S.: Винодельческие провинции — это есть рулез!
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.