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

Пожалуйста, покритикуйте реализацию.

Задание:

Написать пул потоков CThreadPool, обрабатывающий задачи, производные от класса CTask.

Задачи переопределяют функцию virtual void CTask::Execute() = 0;

Пулу при инициализации задается число рабочих потоков:
bool CThreadPool::Init(int _numThreads);

Задачи добавляются для разных клиентов. Задачи одного клиента обрабатываются в том порядке, в котором они были добавлены. Разные клиенты обслуживаются параллельно.
bool CThreadPool::AddTask(int _clientId, CTask* _task);

Не должно быть "повисших" клиентов, даже если клиентов больше, чем потоков.
Т.е. поток должен обрабатывать разных клиентов по-очереди, а не одного до завершения всех его задач.

Реализация(яндекс диск, архив С++ проекта на Visual Studio):

https://yadi.sk/d/PDLZZDPih3QT4

В архиве есть описание хода мыслей



Заранее спасибо за потраченное время.

Здравствуйте, уважаемый Selavi!

Сложно сказать, что именно они считают как слабо.
Лично мне не понравилось, что члены классов никак не выделены:

class CThreadPoolX: public IFinishClientTask
{
...
bool terminated;
...
std::vector<WorkThreadPtr> threads;
std::thread thread;
};

в теле метода их можно запросто перепутать с локальной (автоматической) переменной.

Я бы дал названия членам класса — примерно так (венгерская нотация):

bool m_bTerminated;
std::vector<WorkThreadPtr> m_VectThreads;
std::thread m_thread;

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

AG>Лично мне не понравилось, что члены классов никак не выделены:

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

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

если вкратце — то сделано не то что требовалось. От слова совсем. Необходимо было реализовать класс CThreadPool с двумя методами по десять строк каждый, а вышел какой-то франкенштейн, непонятно что делающий. Ещё раз перечитайте задание, внимательно. И сделайте именно то, что требуется.

A>а вышел какой-то франкенштейн, непонятно что делающий

isn't it what all production code is all about?

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

S>...выполнено слабо
S>К сожалению, добиться обратной связи на тему "что именно слабо" не удалось.

S>Пожалуйста, покритикуйте реализацию.

1. комментариев нет от слова совсем

2.

int Size()
{
std::lock_guard<std::mutex> lock(this->mutex);

return container.size();
}

вместо int должен быть хотя бы size_t

3. TaskDer зачем-то засунут в Task.h

4. Смысл существования task.cpp непонятен, возникает вопрос понимает ли автор, что он делает. (хотя, наверное, это следствие п. 3)

5. использование голых указателей и C++0x в одном проекте. Трусы или крестик.

6.

CWorkThread::CWorkThread(IFinishClientTask *clientTaskFinisher)
: terminated(false)
, thread(&CWorkThread::ThreadFunc, this)
{
this->clientTaskFinisher = clientTaskFinisher;
}

непонятно зачем частично инициализируем обьект в init list, частично в теле конструктора.

7. Класс, в котором есть голые указатели и отсутствие для этого класса copy ctor и operator=

8. \r\n вместо std::endl

9. Библиотечные классы, выводящие debug-данные на консоль без возможности отключения.

10. "не должно быть подвисших клиентов даже если клиентов больше, чем потоков" — требование, которое невозможно соблюсти в текущей формулировке задания и API. Ваш код и комментарии не говорят об этом ни слова, вместо этого вы придумали эвристику, которая не работает.

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

S>...выполнено слабо
S>К сожалению, добиться обратной связи на тему "что именно слабо" не удалось.

S>Пожалуйста, покритикуйте реализацию.

1) слишком много классов, реализация размазана по трем классам, очень сложно читать
2) лишний поток для раскидывания задач, потоки — дорогое удовольствие.
3) отсутствует оповещение об окончании
4) из-за 3) неясно как будет освобождаться task
5) очередь стоило бы сделать lockfree

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

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

AG>>Лично мне не понравилось, что члены классов никак не выделены:

T>Это не проблема, если вы код пишете не в блокноте.
Конечно, такое оформление кода — не проблема, однако это фактор затрудняющий понимание (даже самим автором, через N лет).
Пишу в MSVC, уже более 15 лет. Привычку придерживаться какой-либо нотации, позаимствовал из проектов, выполненных нашей командой для зарубежных Заказчиков (им же передавали и исходники).

T>Плюс, если начинаете путаться в переменных, то, видимо, метод уже слишком большой и его необходимо декомпозировать.
Для production кодов, далеко не всегда имеет смысл декомпозировать метод. Это может привести к излишним сложностям.
В любом случае, метод может занимать более одного экрана.

Спасибо всем за ответы

Большинство замечаний — о мелочах, которые конечно важны, но не чрезмерно. Почти все они относятся к реализации классов, которые играют роль поставщика тестовых данных и которые я писал без должного внимания, ибо в реальном проекте их просто не будет. Но, согласен, стоило бы и поаккуратней.

Вот что меня несколько удивило:

antropolog писал:

A> если вкратце — то сделано не то что требовалось. От слова совсем. Необходимо было реализовать класс CThreadPool с двумя методами по десять строк каждый, а вышел какой-то франкенштейн, непонятно что делающий. Ещё раз перечитайте задание, внимательно. И сделайте именно то, что требуется.

Нигде в задании нет ни слова о 2х методах по десять строк) Но да бог с ним, я не об этом...

Честное слово, не представляю как можно реализовать ТЗ в 2х десятках строчек.
Для выполнения условия "Не должно быть "повисших" клиентов, даже если клиентов больше, чем потоков. Т.е. поток должен обрабатывать разных клиентов по-очереди, а не одного до завершения всех его задач" придется в любом случае вести список клиентов и на его основе вычислять следующее задание для свободного потока.

Или мсье может предложить алгоритм попроще?


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

A>1) слишком много классов, реализация размазана по трем классам, очень сложно читать
A>2) лишний поток для раскидывания задач, потоки — дорогое удовольствие.
A>3) отсутствует оповещение об окончании
A>4) из-за 3) неясно как будет освобождаться task
A>5) очередь стоило бы сделать lockfree

1) Сложно? Имо, наоборот, это как раз суть ООП — создавать абстракции и раскидывать их по классам. Если взять класс CThreadPoolX и просмотреть его методы, то становится однозначно понятно, что он делает. Я много раз видел, как люди запихивают все в 1 класс и это кошмар для понимания
2) Согласен, но без него будет подвисать пользователь пула потоков, что навряд ли правильно. Впрочем, тут вопрос производительности
3) Об окончании чего?
4) Не совсем понял о чем речь. Все task обернуты в shared_ptr и корректно освобождаются после удаления их из очередей. Есть, конечно, вариант, что они освободятся на стороне пользователя пула потоков, но знаете, это же все таки тестовое задание, а не реальная разработка.
5) Да, пожалуй, Вы правы.

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

AG>Здравствуйте, уважаемый Selavi!

AG>Сложно сказать, что именно они считают как слабо.
AG>Лично мне не понравилось, что члены классов никак не выделены:

члены класса выделяются через this-> в местах, где есть вариант перепутать их с локальной переменной

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

S>...выполнено слабо
S>К сожалению, добиться обратной связи на тему "что именно слабо" не удалось.

Просмотрел поверхностно, возможно, чего-то не заметил.

Вот это — очень и очень плохо:

TaskPtr *task = new TaskPtr(new CTaskDer(sec));

Коль скоро у тебя API на shared_ptr, то можно было бы ожидать чего-то такого:

TaskPtr task;
task.reset(new CTaskDer(sec));
pool->AddTask(ccc - 48, task);

Но если так сделать, тестовый пример падает. Дальше потянулась ниточка, которую я полностью распутывать не стал, увидев, что задание преждевременно разрушается в методе CThreadPoolX::ProcessFreeThreads(). Увидь я такое у коллег, первой мыслью было бы: "Что они курили?!"

Получается, что ты не смог наладить управление жизненным циклом, поэтому, не смотря на то, что пул оперирует shared_ptr-ами, тебе приходится обязательно держать их где-то вовне пула. Иными словами, нужно городить какую-то внешнюю схему для контроля за заданиями, уже отданными пулу. Теоретически можно, но зачем тогда строить пул на shared_ptr? Оставил бы там обычные указатели.

То есть действительно, реализация слабая.

Потом, ты не точно следуешь заданию. Так, в задании написано:

bool CThreadPool::AddTask(int _clientId, CTask* _task);

Обрати внимание, какой здесь показан указатель на CTask.

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

this->clientTaskFinisher = clientTaskFinisher;

Ещё немного по реализации.

1) Лучше было бы сделать двухфазную инициализацию/разрушение: конструктор + init и shutdown + деструктор. Меня бы напрягло отсутствие такой "автоматической" реакции у интервьюируемого.

2) Ключевое слово __interface? C++11? O'rly?

3) Комментарии. Это вообще больной вопрос. Ребята, ну не надо издеваться над интервьюерами, а? Или пусть будет паритет: у одних код без комментариев, у других ответ без разъяснений.

4) Тестовый пример лучше бы оформить в виде автоматизированных тестов, где, например, проверялась бы работа пула с заданиями нулевой длительности, с заданиями, выбрасывающими исключения и т.п. Диалоговый режим теста — это не тест, это demo.

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

>Честное слово, не представляю как можно реализовать ТЗ в 2х десятках строчек.
поэтому ты и реализовал франкенштейна. А надо было всего-лишь реализовать простой паттерн — привязку продюсера к конкретному потоку. В данном случае это делается отдельной очередью на каждый поток, и помещением таски в очередь с индексом = taskID % threadNum

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

S>Или мсье может предложить алгоритм попроще?

На вскидку:

— Каждое задание снабжается маркером клиента;
— Свободный поток получает маркер не обслуживаемого клиента и поочерёдно выбирает соответствующие задания из общей входной очереди;
— Если свободного потока нет, добавляем маркер очередного клиента одному из занятых потоков (здесь можно вставить какую-нибудь несложную эвристику), после чего он начинает выбирать задания всех привязанных к нему клиентов.

Диспетчерский метод (AddTask) по сути только разбрасывает маркеры клиентов по занятым или свободным потокам. Выборка заданий из очереди — ответственность WorkingThread. Будет небольшое "бутылочное горло" с совместным доступом к очереди заданий, но именно, что небольшое, вполне приемлемое для тестового задания.

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

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

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

>>Честное слово, не представляю как можно реализовать ТЗ в 2х десятках строчек.
A>поэтому ты и реализовал франкенштейна. А надо было всего-лишь реализовать простой паттерн — привязку продюсера к конкретному потоку. В данном случае это делается отдельной очередью на каждый поток, и помещением таски в очередь с индексом = taskID % threadNum

Сразу скажу, что отдельная очередь на каждый поток — самое плохое решение.
Почему?
Причин несколько:
1) Может возникнуть ситуация, когда у одного потока в очереди есть одно или больше заданий, хотя в это время же соседний поток свободен
2) Будут возникать ситуации параллельного выполнения заданий от 1го клиента. Это нарушение условия ТЗ и чтобы его исправить понадобится большем, чем 20 строк кода.
3) Чтобы реализовать равномерное распределение серверного времени по клиентам в Вашем варианте придется написать еще 20+ строчек кода

Вывод: Мсье торопится

Здравствуйте, Геннадий Васильев, Вы писали:

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

S>>Или мсье может предложить алгоритм попроще?

ГВ>На вскидку:

ГВ>- Каждое задание снабжается маркером клиента;
ГВ>- Свободный поток получает маркер не обслуживаемого клиента и поочерёдно выбирает соответствующие задания из общей входной очереди;
ГВ>- Если свободного потока нет, добавляем маркер очередного клиента одному из занятых потоков (здесь можно вставить какую-нибудь несложную эвристику), после чего он начинает выбирать задания всех привязанных к нему клиентов.


Этот алгоритм не годится по причинам, описанным в моем посте выше

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

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


S>1) Может возникнуть ситуация, когда у одного потока в очереди есть одно или больше заданий, хотя в это время же соседний поток свободен
да. может. но во-первых, если говорить о реальном мире, то это обычно самая оптимальная стратегия, а во-вторых, об оптимизации стратегии планировщика можно написать диссертацию, и это явно не для тестового задания

S>2) Будут возникать ситуации параллельного выполнения заданий от 1го клиента. Это нарушение условия ТЗ и чтобы его исправить понадобится большем, чем 20 строк кода.
нет не будут, клиент с одним id будет всегда попадать в один и тот же поток

S>3) Чтобы реализовать равномерное распределение серверного времени по клиентам в Вашем варианте придется написать еще 20+ строчек кода
где в т.з. что-то о равномерном распределении?

S>Вывод: Мсье торопится
малыш, ты бы лучше прочёл хоть одну бы книгу про многопоточное программирование

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

S>Этот алгоритм не годится по причинам, описанным в моем посте выше

S>Основная проблема — мы не можем "на лету" вычислять следующее задание для потока, поскольку нам не известно время выполнения текущего задания.

А мы и не вычисляем его "на лету". Рабочий поток заканчивает выполнение очередного задания и переходит к поиску следующего.

S>При таком способе мы придем к тому, что одни клиенты будут ждать свободный поток дольше других

И в чём проблема? Это специфика ограничений задачи, не надо городить лишнего.

Допустим, у тебя пул на 3 потока, все они сейчас заняты клиентами с номерами, соответственно, 1..3. Появляется четвёртый. Куда его девать? Простейшее решение: подсунуть задания 4-го клиента первому же потоку, допустим, это поток номер 1. Тогда он станет выполнять задания в порядке: 1,4,1,4... Можно немного усложнить решение и отложить назначение 4-го клиента до момента, пока где-нибудь не завершится очередное задание — тогда поток, который первый начал поиск нового задания, заодно получит и нового клиента.

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

IRL можно накрутить диспетчеризацию в зависимости от загруженности потоков, статистики сложности заданий от клиентов и всё, что угодно.

Здравствуйте, Геннадий Васильев, Вы писали:

ГВ>Вот это — очень и очень плохо:

ГВ>

TaskPtr *task = new TaskPtr(new CTaskDer(sec));

ГВ>Ещё немного по реализации.

ГВ>1) Лучше было бы сделать двухфазную инициализацию/разрушение: конструктор + init и shutdown + деструктор. Меня бы напрягло отсутствие такой "автоматической" реакции у интервьюируемого.

ГВ>2) Ключевое слово __interface? C++11? O'rly?

ГВ>3) Комментарии. Это вообще больной вопрос. Ребята, ну не надо издеваться над интервьюерами, а? Или пусть будет паритет: у одних код без комментариев, у других ответ без разъяснений.

ГВ>4) Тестовый пример лучше бы оформить в виде автоматизированных тестов, где, например, проверялась бы работа пула с заданиями нулевой длительности, с заданиями, выбрасывающими исключения и т.п. Диалоговый режим теста — это не тест, это demo.


1)
Да, я мог бы реализовать конструктор копирования в CTask и создавать копии заданий работы пула потоков (кстати, тоже можно придраться за лишние операции и расход памяти)
Да, я мог бы реализовать корректное хранение и удаление заданий вне пула потоков

Но есть же какое то разумное ограничение на время для тестового задания? Или мне надо было писать промышленную версию? Я просто не стал морочиться и кстати, написал это и в комменте к строчке и в описании к ТЗ. Почему то Вы на это не обратили внимания.

2) Что такого плохого в this->clientTaskFinisher = clientTaskFinisher?

3) Чем же лучше 2хфазная инициализацию/разрушение?

4) Вам не нравится использование интерфейсов в С++? Раздражает constructor injection? Что не так?

5) Да, с комментариями больной вопрос. Лично я придерживаюсь идеи дядюшки Боба о том, что комментарии засоряют код и нужно комментировать названиями методов и переменных. Считаю, что код в ТЗ вполне понятен и не требует лишней прозы.

6) Может и так, не буду спорить, хотя это снова мелочь.


Похоже на мелкие придирки, которые каждый может изобрести на свой вкус

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

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

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


S>>1) Может возникнуть ситуация, когда у одного потока в очереди есть одно или больше заданий, хотя в это время же соседний поток свободен
A>да. может. но во-первых, если говорить о реальном мире, то это обычно самая оптимальная стратегия, а во-вторых, об оптимизации стратегии планировщика можно написать диссертацию, и это явно не для тестового задания

S>>2) Будут возникать ситуации параллельного выполнения заданий от 1го клиента. Это нарушение условия ТЗ и чтобы его исправить понадобится большем, чем 20 строк кода.
A>нет не будут, клиент с одним id будет всегда попадать в один и тот же поток

S>>3) Чтобы реализовать равномерное распределение серверного времени по клиентам в Вашем варианте придется написать еще 20+ строчек кода
A>где в т.з. что-то о равномерном распределении?

S>>Вывод: Мсье торопится
A>малыш, ты бы лучше прочёл хоть одну бы книгу про многопоточное программирование


Опять какая то демагогия, которую я могу бы разбить в 2 счета, но, судя по всему, мсье — обычный хам, поэтому говорить с ним не хочется.

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

S>Опять какая то демагогия, которую я могу бы разбить в 2 счета, но, судя по всему, мсье — обычный хам, поэтому говорить с ним не хочется.

с воинствующим невежеством общаться вообще нет резона. ариведерчи.