Комментарии 51
Можно наверное было воспользоваться секцией Q&A, по сабжу — глянул только в Task и не совсем понял зачем Execute создает тред и запускает выполнение в нем? Вам в же задании написали, что интерфейс Task предоставляет только метод Execute и никаких ивентов.
Событие по идее надо было сделать internal, просто проглядел. Формально это бы не нарушило требований по интерфейсу класса. Но, думаю, это была далеко не главная проблема. Ниже люди хорошо описали бОльшую проблему моего кода — фактическое отсутствие ПУЛА потоков. Потоки повторно не использовались.
Что касается секции Q&A, возможно, вы правы. Просто человек я на Хабре новый и ещё не до конца разобрался с её анатомией.
Создание потока — операция очень затратная. Пул потоков нужен для того, чтобы хранить в нём несколько уже созданных потоков, обычно по числу ядер в системе. При поступлении задачи, она помещается в очередь. Когда какой-либо поток прекращает работу, он берёт очередную задачу из очереди и начинает её выполнение. В статье ParallelExtensionsExtras Tour — #7 — Additional TaskSchedulers и коде из проекта ParallelExtensionsExtras в Samples for Parallel Programming with the .NET Framework есть пример QueuedTaskScheduler, который выполняет описанные в задании требования.
Хотя я к Связному не имею отношения, попробую написать, что вы сделали не так.
Задача действительно решена неверно, у вас нет никакого ThreadPool'а.
Самая главная ошибка: каждая задача у вас выполняется в новом потоке:
Вместо этого необходимо при создании пула завести aConcurrentTaskNumber потоков, которые будут выполнять задачи. Поток sheduler'а в таком случае не нужен вообще, все постановки задач в очередь выполняются синхронно.
Если бы вы были знакомы с концепцией .NET ThreadPool (и вообще паттерном thread pool), проблемы бы не было.
Задача действительно решена неверно, у вас нет никакого ThreadPool'а.
Самая главная ошибка: каждая задача у вас выполняется в новом потоке:
Thread lTaskThread =
new Thread(
() =>
{
// Выполнить задачу.
TaskBody();Вместо этого необходимо при создании пула завести aConcurrentTaskNumber потоков, которые будут выполнять задачи. Поток sheduler'а в таком случае не нужен вообще, все постановки задач в очередь выполняются синхронно.
Если бы вы были знакомы с концепцией .NET ThreadPool (и вообще паттерном thread pool), проблемы бы не было.
Да, тут грешен. Действительно, упустил из внимания, что создание потока — операция затратная. Думаю, это главная проблема.
На самом деле есть более общий шаблон — resource pool, о нём знаю и использую. Принцип общий: есть дорогой ресурс, используй его повторно. Используется, например, с подключениями к базе данных.
Что касается того, что все постановки задач можно делать синхронно, тут можно поспорить. Планировщик в данном случае у меня не добавляет задачи в очередь, а извлекает их из неё по мере освобождения потоков. Задачи выполняются в отдельных потоках и завершаются в произвольные моменты времени. После того, как запущенно максимальное количество потоков, планировщик должен остановиться и подождать окончания одного из них. Если отдельного потока планировщика не будет, будут останавливаться главный поток. Или я неправильно понял вашу мысль?
На самом деле есть более общий шаблон — resource pool, о нём знаю и использую. Принцип общий: есть дорогой ресурс, используй его повторно. Используется, например, с подключениями к базе данных.
Что касается того, что все постановки задач можно делать синхронно, тут можно поспорить. Планировщик в данном случае у меня не добавляет задачи в очередь, а извлекает их из неё по мере освобождения потоков. Задачи выполняются в отдельных потоках и завершаются в произвольные моменты времени. После того, как запущенно максимальное количество потоков, планировщик должен остановиться и подождать окончания одного из них. Если отдельного потока планировщика не будет, будут останавливаться главный поток. Или я неправильно понял вашу мысль?
Сами рабочие потоки пусть из очереди и извлекают, им для этого поток-планировщик не нужен.
Потоки будут спать, пока не появится новая задача в очереди.
Задача появилась — поток проснулся — выполнил очередную задачу — если есть еще задачи, выполнил их — опять заснул.
Пользователь класса просто добавляет потоки в очередь и устанавливает событие, которое разбудит рабочий поток.
Задача появилась — поток проснулся — выполнил очередную задачу — если есть еще задачи, выполнил их — опять заснул.
Пользователь класса просто добавляет потоки в очередь и устанавливает событие, которое разбудит рабочий поток.
Наверное скоро будет for паттерн, if паттерн и т.п. Мне одному кажеться, что за вся эта мешура мешает видеть суть?
Очевидно, мне одному так кажеться, ну или мы сильно в меньшинстве.
В качестве реабилитации, немного конструктива, то что мне кажется не правильным, помима факта неверного решения.
FindNextTaskUsingPriorityRules — этого метода быть не должно, зачем такие сложности и при этом нет никакой гибкости. LINQ там лишнее, простой проход в цикле был более уместен, и находил бы задачу за один проход, и был более читабельным на мой взгляд. Так еще потом при удалении задачи из списка в методе DequeueTask, потребуется еще один проход.
Бегло просматривая, встретил еще пару мест, где происходит примерно тоже самое.
Можно было организовать три очереди, чтобы исключить поиск полностью.
Или например то, что задача запускает поток, да возможно что автор написал так из-за того что не разобрался что такое ThreadPool, но тем не менее: задача — ее выполняют, пул потоков — тут даже нечего добавить. Логично же, что потоки должны быть в пуле?!
И зачем здесь патерны, вообще и в частности?! Фабрика задач, упростит понимание кода — не думаю. Или пул нужно обязательно делать синглтоном — нет, статические методы решают задачу в данном случае.
Я считаю патерны полезны в основном как средство как средство коммуникации между разработчиками.
В качестве реабилитации, немного конструктива, то что мне кажется не правильным, помима факта неверного решения.
FindNextTaskUsingPriorityRules — этого метода быть не должно, зачем такие сложности и при этом нет никакой гибкости. LINQ там лишнее, простой проход в цикле был более уместен, и находил бы задачу за один проход, и был более читабельным на мой взгляд. Так еще потом при удалении задачи из списка в методе DequeueTask, потребуется еще один проход.
Бегло просматривая, встретил еще пару мест, где происходит примерно тоже самое.
Можно было организовать три очереди, чтобы исключить поиск полностью.
Или например то, что задача запускает поток, да возможно что автор написал так из-за того что не разобрался что такое ThreadPool, но тем не менее: задача — ее выполняют, пул потоков — тут даже нечего добавить. Логично же, что потоки должны быть в пуле?!
И зачем здесь патерны, вообще и в частности?! Фабрика задач, упростит понимание кода — не думаю. Или пул нужно обязательно делать синглтоном — нет, статические методы решают задачу в данном случае.
Я считаю патерны полезны в основном как средство как средство коммуникации между разработчиками.
А не проще ли взять .NET4? Там за вас уже всё сделали.
И паралельные циклы, и ограниченые пулы, и очереди с приоритетами, и массу всего что вы пытаетесь сделать.
И паралельные циклы, и ограниченые пулы, и очереди с приоритетами, и массу всего что вы пытаетесь сделать.
Вы, видимо, не поняли. Это тестовое задание, в котором нужно продемонстрировать своё понимание принципов работы с многопоточностью. То есть без использования базовых объектов не обойтись. А все вкусности .NET 4.0 я и сам использую с удовольствием, просто в данном контексте они бы не сработали в качестве примера, как я понял.
Странная политика у связного. В принципе, это по крайней мере не быдло код, с вами могли бы побеседовать, вытянуть проблемы, о которых вам здесь написали. Уж, наверняка, сами бы догадались по фразе «Создание потока — операция очень затратная.»
Я общался только с кадровиком. О личных качествах (или подходах к найму сотрудников) заинтересованных лиц в проекте я ничего не знаю. Возможно, у них стоит очередь из соискателей один другого круче. Возможно, у них настолько критичный проект, что мои ошибки уронят какой-нибудь самолёт с логотипов Связной на борту. :) Кто знает?
Что-то многовато блокировок. Да и вообще кода много. Кажется, что можно все это сделать проще. Например, блокировку на IsStopped можно заменить обычным volatile полем. Очередь с приоритетами реализовать отдельно от логики собственно ThreadPool'а (тут можно кстати и подумать, сделать ли её потокобезопасной или же обеспечивать потокобезопасность в вызывающем коде). А при остановке всех задач у потоков должны вызываться Thread.Join(). Interlocked-методы тоже неясно зачем (если уже используются мониторы и event'ы). В общем, имхо, вы усложнили )
А какие конкретно претензии по коду вам предъявили кроме «это не то, что мы телепатировали»? Да, и странно, что вам это написал кадровик, а не технический специалист.
А какие конкретно претензии по коду вам предъявили кроме «это не то, что мы телепатировали»? Да, и странно, что вам это написал кадровик, а не технический специалист.
По первому абзацу сейчас ничего сказать не могу, надо осмыслять. :) Но в любом случае, спасибо.
По поводу общения. Ничего кроме этого не предъявили, если понимать вашу фразу о телепортации, как парафраз «это далеко от истины». Да, я тоже считаю, что правила хорошего тона требуют ответа от технического специалиста, но, видимо, у них есть свои резоны поступать так. Может быть они настолько ценят своё время, что code review присланных решений у них идёт отдельной строчкой в прейскуранте. А может быть они просто используют кадровиков, как щит. Я не знаю.
По поводу общения. Ничего кроме этого не предъявили, если понимать вашу фразу о телепортации, как парафраз «это далеко от истины». Да, я тоже считаю, что правила хорошего тона требуют ответа от технического специалиста, но, видимо, у них есть свои резоны поступать так. Может быть они настолько ценят своё время, что code review присланных решений у них идёт отдельной строчкой в прейскуранте. А может быть они просто используют кадровиков, как щит. Я не знаю.
Связной эту задачу уже как минимум два года эту всем по сети рассылает.
И непонятна их цель, если такое решение их не устраивает да ещё на ту з/пл, которую они предлагают.
И непонятна их цель, если такое решение их не устраивает да ещё на ту з/пл, которую они предлагают.
Ну, судя по комментариям, я действительно сделал как минимум одну серьёзную ошибку.
Позвольте полубопытсвовать, во сколько Связной оценивает специалиста который готов решать им подобные задачи?
Цена договорная. :) Посмотрите на hh.ru текущие предложения, они пишут зарплаты (правда gross, а не net). Они эту задачу всем подряд дают уже не первый год, как я понимаю.
www.superjob.ru/rabota/vacancy-11794787.html
Тут написано, что от 138 тысяч в месяц.
Тут написано, что от 138 тысяч в месяц.
Цена договорная. :) Посмотрите на hh.ru текущие предложения, они пишут зарплаты (правда gross, а не net). Они эту задачу всем подряд дают уже не первый год, как я понимаю.
Было бы очень интересно, если бы автор решил озвученные в комментариях недочеты и снова отправил в Связной обновленную версию решения. А когда все решится, то было бы интересно почитать продолжение поста (как мне видится они могут воспринять этот порыв как четкое желание работать в Связном и принять Вас на работу).
НЛО прилетело и опубликовало эту надпись здесь
Мы учимся постоянно — такая специализация в ИТ.
Если в Связном это понимают, то они должны, по идее, оценить автора по достоинству, ибо он готов, способен развиваться и двигаться дальше, достигая поставленных задач, пусть и не сразу. Думаю это важно.
Понятное дело — всем нужны уже «готовые» специалисты, однако не всегда можно найти такого человека. Порой смешно становится видя вакансии с жизненным циклом более года — не проще ли взять пусть не готового, но способного к саморазвитию человека?!
Если в Связном это понимают, то они должны, по идее, оценить автора по достоинству, ибо он готов, способен развиваться и двигаться дальше, достигая поставленных задач, пусть и не сразу. Думаю это важно.
Понятное дело — всем нужны уже «готовые» специалисты, однако не всегда можно найти такого человека. Порой смешно становится видя вакансии с жизненным циклом более года — не проще ли взять пусть не готового, но способного к саморазвитию человека?!
НЛО прилетело и опубликовало эту надпись здесь
Уважаемый, ты так пишешь, как-будто сам никогда не делаешь ошибок. Я же не только признаю за собой возможность ошибиться, но и не стесняюсь показать это, спросить у других совета. Сделай, пожалуйста, выводы.
НЛО прилетело и опубликовало эту надпись здесь
Вы затронули тему моего опыта. Опыта у меня поболее Вашего, скорее всего, будет. Заявить, что я не знаю, что такое ThreadPool, было весьма опрометчиво с Вашей стороны. Я никогда не писал свой ThreadPool, это так, но я использовал готовый множество раз. А теперь представьте, что Вам дают задание написать свой GC, пусть и за большее время. Справитесь, не забыв ни об одном свойстве оригинала? Вряд ли.
Тем более Вы не написали ни одного замечания по существу, лишь воспользовались тем, что уже разглядели другие.
Тем более Вы не написали ни одного замечания по существу, лишь воспользовались тем, что уже разглядели другие.
Как человеку новому здесь, посоветую: никогда не переходите на личности и используйте обращение на «вы», а не на «ты». Это позволит вам сохранить карму в плюсе и полноценно участвовать в жизни сообщества.
Спасибо за совет, добрый человек, но не я начал. Пришлось открыто высказать свои претензии в ответ на открыто высказанные претензии ко мне. Посмотрите внимательно и Вы поймёте о чём я говорю.
Я тоже считаю эту ветку комментариев откровенным флеймом. Antelle не сказал ничего по существу вопроса, лишь высказал своё опрометчивое мнение о моих качествах во всеуслышание. Так что тут пришлось наплевать на карму.
Я тоже считаю эту ветку комментариев откровенным флеймом. Antelle не сказал ничего по существу вопроса, лишь высказал своё опрометчивое мнение о моих качествах во всеуслышание. Так что тут пришлось наплевать на карму.
НЛО прилетело и опубликовало эту надпись здесь
На самом деле мне не понравилась фраза «человека, только вчера узнавшего, что такое ThreadPool». «Только вчера» узнать об этом мог лишь человек обладающий минимальным опытом. Поэтому и говорю об опыте. Да, я упустил из внимания это требования, наличие которого следует из названия класса, сконцентрировавшись на явных требованиях, указанных в описании задачи. Эту свою ошибку я признал и неоднократно, посмотрите в моих комментариях. Зачем было делать намёк на мой опыт, я не понимаю, поэтому и вскипятился. Надеюсь, я достаточно ясно выразил свою позицию, и мы больше не будем разводить флейм по этому вопросу.
Извинения Antelle принимаю, лишь хотел объяснить, что конкретно меня задело, чтобы человек не делал эту ошибку в будущем по отношению к кому бы то ни было.
Извинения Antelle принимаю, лишь хотел объяснить, что конкретно меня задело, чтобы человек не делал эту ошибку в будущем по отношению к кому бы то ни было.
да ладно, не напрягайтесь, здесь часто говорят очевидные вещи и выпадают из контекста.
но и полезного много можно услышать.
рекомендую игнорировать то что не нравится и отвечать только там где есть что-то полезное.
но и полезного много можно услышать.
рекомендую игнорировать то что не нравится и отвечать только там где есть что-то полезное.
Уже не напрягаюсь, спасибо. Когда будет возможность плюсовать/минусовать буду просто минусовать в таких случаях.
Работаю над ошибками. Сейчас обновлю статью, если это можно сделать на Хабре, добавлю исправленную реализацию.
Работаю над ошибками. Сейчас обновлю статью, если это можно сделать на Хабре, добавлю исправленную реализацию.
Вот, честное слово, не пойму, за что минусуете предыдущее моё высказывание. Я же не сказал, что буду «срать в тапки» «гадить в карму». Неужели это так крамольно минусовать неконструктивные комментарии?
Да что вы дерзкий-то такой.
Вы пришли за советом, вам его дали, попутно покритиковали — но вы же сами попросили, не мы же к вам пришли в дом и начали код обсуждать.
А вы тут сразу на «ты» перешли, минусами угрожаете, считаете себя в области GC. Ну расскажите мне про GC в Go например, там он довольно необычен, но Вы-то специалист, ничего не упускаете из виду.
Вы пришли за советом, вам его дали, попутно покритиковали — но вы же сами попросили, не мы же к вам пришли в дом и начали код обсуждать.
А вы тут сразу на «ты» перешли, минусами угрожаете, считаете себя в области GC. Ну расскажите мне про GC в Go например, там он довольно необычен, но Вы-то специалист, ничего не упускаете из виду.
За конструктивную критику я уже высказал благодарности, за неконструктивную поругался. Думаю, Вы бы сделали тоже самое на моём месте. Читайте обновлённый вариант статьи.
Давайте уже, действительно, заканчивать флейм, в этом Вы правы. Я продолжать точно не буду, так что можете сказать своё последнее веское слово, и закончим. :)
Давайте уже, действительно, заканчивать флейм, в этом Вы правы. Я продолжать точно не буду, так что можете сказать своё последнее веское слово, и закончим. :)
забей,
т.к. к программированию данный вопрос не имеет отношения, ибо ты получил ответ HR, а не от тим-лидера, или архитектора и даже не от разработчика…
это даже не собеседование — ты не получил фидбека от человека, который давал оценку твоему решению…
p.s. вспоминаю собеседования в сибоссе в начале 2000х которые были апофеозом страстей и тщеславия топ-менеджмента… и теперь привели контору в глубокую ж…
т.к. к программированию данный вопрос не имеет отношения, ибо ты получил ответ HR, а не от тим-лидера, или архитектора и даже не от разработчика…
это даже не собеседование — ты не получил фидбека от человека, который давал оценку твоему решению…
p.s. вспоминаю собеседования в сибоссе в начале 2000х которые были апофеозом страстей и тщеславия топ-менеджмента… и теперь привели контору в глубокую ж…
Да я и не парюсь на тему работы в Связном, просто хотелось дойти до истины, что у меня не так в коде, а для этого лучше всего подходит коллективный разум. Заодно получилась неплохая статья. Жаль, что кто-то анонимно накакал в карму, так что пока что писать не смогу. Видимо, это тут в порядке вещей.
За напоминание о Сибоссе отдельное спасибо. Как раз недавно вспоминал название этой фирмы. Тоже рассматривал их предложение году этак в 2003. Но мне, для принятия решения даже не соваться в эту кантору, хватило простого взгляда на здание, где располагался их офис, и пары разговоров с работниками, вышедшими покурить.
За напоминание о Сибоссе отдельное спасибо. Как раз недавно вспоминал название этой фирмы. Тоже рассматривал их предложение году этак в 2003. Но мне, для принятия решения даже не соваться в эту кантору, хватило простого взгляда на здание, где располагался их офис, и пары разговоров с работниками, вышедшими покурить.
Вот эти конструкции ни к чему, как мне кажется. Разве только volatile на IsStopped.
lock (mIsStoppedLock)
{
if (IsStopped)
цитата из сэмплов, что в комплекте с 10ой студией.
while (!_shouldStop)
{
…
}
lock (mIsStoppedLock)
{
if (IsStopped)
цитата из сэмплов, что в комплекте с 10ой студией.
while (!_shouldStop)
{
…
}
Вот я смотрю на итоговый результат и что сразу же вижу, что вызов Task.Execute(); находится внутри всех блокировок, решение банально не работает. Сейчас илллюзия работы, так как время задач слишком маленькое, меняем Thread.Sleep(lRandom.Next(50, 500)); на Thread.Sleep(10000)); и видим трогательное исполнение 10-секундных задач фактически в одном потоке.
Первоначальное решение не видел, но сейчас это действительно «совсем не то». Но это не простая тема, такие ошибки даже в серьезных книгах есть, вот, например, мой вопрос на Stackoverflow про нерабочий пример в Accelerated C# 2008.
Первоначальное решение не видел, но сейчас это действительно «совсем не то». Но это не простая тема, такие ошибки даже в серьезных книгах есть, вот, например, мой вопрос на Stackoverflow про нерабочий пример в Accelerated C# 2008.
Зарегистрируйтесь на Хабре, чтобы оставить комментарий
Тестовое задание для Связного FixedThreadPool на C#. Что здесь не так? UPD