Сколько работы я должен поместить в оператор блокировки?

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

Глядя на код из предыдущей версии, я вижу это:

        static Object _workerLocker = new object();
        static int _runningWorkers = 0;
        int MaxSimultaneousThreads = 5;

        foreach(int SomeObject in ListOfObjects)
        {
            lock (_workerLocker)
            {
                while (_runningWorkers >= MaxSimultaneousThreads)
                {
                    Monitor.Wait(_workerLocker);
                }
            }

            // check to see if the service has been stopped. If yes, then exit
            if (this.IsRunning() == false)
            {
                break;
            }

            lock (_workerLocker)
            {
                _runningWorkers++;
            }

            ThreadPool.QueueUserWorkItem(SomeMethod, SomeObject);

        }

Логика кажется ясной: дождитесь места в пуле потоков, убедитесь, что служба не была остановлена, затем увеличьте счетчик потоков и поставьте в очередь работу. _runningWorkersуменьшается внутри SomeMethod()внутри lockоператора, который затем вызывает Monitor.Pulse(_workerLocker).

Мой вопрос: есть ли какая-то польза от группировки всего кода внутри одного lock, например:

        static Object _workerLocker = new object();
        static int _runningWorkers = 0;
        int MaxSimultaneousThreads = 5;

        foreach (int SomeObject in ListOfObjects)
        {
            // Is doing all the work inside a single lock better?
            lock (_workerLocker)
            {
                // wait for room in ThreadPool
                while (_runningWorkers >= MaxSimultaneousThreads) 
                {
                    Monitor.Wait(_workerLocker);
                }
                // check to see if the service has been stopped.
                if (this.IsRunning())
                {
                    ThreadPool.QueueUserWorkItem(SomeMethod, SomeObject);
                    _runningWorkers++;                  
                }
                else
                {
                    break;
                }
            }
        }

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

Единственные другие места, где _workerLockerблокируется, находятся внутри SomeMethod(), и только с целью уменьшения _runningWorkers, а затем за пределами, foreachчтобы ждать, пока число не станет _runningWorkersравным нулю, прежде чем регистрироваться и возвращаться.

Спасибо за любую помощь.

РЕДАКТИРОВАТЬ 4/8/15

Спасибо @delnan за рекомендацию использовать семафор. Код становится:

        static int MaxSimultaneousThreads = 5;
        static Semaphore WorkerSem = new Semaphore(MaxSimultaneousThreads, MaxSimultaneousThreads);

        foreach (int SomeObject in ListOfObjects)
        {
            // wait for an available thread
            WorkerSem.WaitOne();

            // check if the service has stopped
            if (this.IsRunning())
            {
                ThreadPool.QueueUserWorkItem(SomeMethod, SomeObject);
            }
            else
            {
                break;
            }
        }

WorkerSem.Release()называется внутри SomeMethod().

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

Между конца while (_runningWorkers >= MaxSimultaneousThreads)и _runningWorkers++, вообще ничего может случиться , потому что код сдачи и повторно получают блокировку между ними. Например, поток A может получить блокировку в первый раз, подождать, пока какой-нибудь другой поток выйдет из нее, а затем выйти из цикла и lock. Затем он выгружается, и поток B входит в изображение, также ожидая места в пуле потоков. Потому что сказал другой поток бросить курить, есть в комнате , так что не очень долго ждать вообще. И поток A, и поток B теперь _runningWorkersработают в некотором порядке, каждый из которых увеличивает и начинает свою работу.

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

Второй фрагмент исправляет эту проблему, насколько я вижу. Менее инвазивным изменением для устранения проблемы может быть размещение ++_runningWorkersсразу после whileпросмотра внутри первого оператора блокировки.

Теперь о правильности, а как насчет производительности? Это трудно сказать. Обычно блокировка на более длительное время («грубо») препятствует параллелизму, но, как вы говорите, это необходимо сбалансировать с накладными расходами из-за дополнительной синхронизации мелкозернистой блокировки. Как правило, единственным решением является сравнительный анализ и осознание того, что существует больше опций, чем «блокировать все везде» и «блокировать только минимум». Существует множество шаблонов, примитивов параллелизма и потоковых структур данных. Например, похоже, что именно для этого были созданы семафоры приложений, поэтому рассмотрите возможность использования одного из них вместо этого свернутого вручную счетчика с ручной блокировкой.

ИМХО, вы задаете не тот вопрос - вас должны заботить не столько компромиссы эффективности, сколько правильность.

Первый вариант обеспечивает _runningWorkersдоступ только во время блокировки, но он пропускает случай, когда _runningWorkersдругой поток может измениться в промежутке между первой блокировкой и второй. Честно говоря, код смотрит на меня, если кто-то вслепую блокирует все точки доступа, _runningWorkersне задумываясь о последствиях и потенциальных ошибках. Может быть, у автора были некоторые суеверные опасения по поводу выполнения breakзаявления внутри lockблока, но кто знает?

Таким образом, вы должны использовать второй вариант не потому, что он более или менее эффективен, а потому, что он (надеюсь) более правильный, чем первый.

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

Сколько работы я должен поместить в оператор блокировки?

Давайте начнем со стандартного совета, на который вы намекаете и на который ссылается Делнан в последнем абзаце принятого ответа:

• Делайте как можно меньше работы, блокируя определенный объект. Блокировки, которые удерживаются в течение длительного времени, могут быть предметом спора, и спор идет медленно. Обратите внимание, что это означает, что общий объем кода в конкретной блокировке и общий объем кода во всех операторах блокировки, которые блокируют один и тот же объект, являются релевантными.

• Имейте как можно меньше блокировок, чтобы снизить вероятность взаимоблокировок (или блокировок).

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

Что мы можем сделать из того факта, что лучший стандартный совет совершенно противоречив? Мы получаем на самом деле хороший совет:

• Не ходи туда во-первых. Если вы делитесь памятью между потоками, вы открываете себя миру боли.

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

Если вы абсолютно уверены, что должны использовать низкоуровневые примитивы параллелизма, такие как потоки или семафоры, то используйте их для создания высокоуровневой абстракции, которая захватывает то, что вам действительно нужно. Вы, вероятно, обнаружите, что абстракция более высокого уровня - это что-то вроде «выполнить задачу асинхронно, которая может быть отменена пользователем», и, эй, TPL уже поддерживает это, так что вам не нужно выполнять свою собственную. Вы, вероятно, обнаружите, что вам нужно что-то вроде поточной безопасной ленивой инициализации; не катите свое собственное использование Lazy<T>, которое было написано экспертами. Используйте безопасные коллекции (неизменяемые или иные), написанные экспертами. Переместите уровень абстракции вверх как можно выше.