Quanto trabalho devo colocar dentro de uma declaração de bloqueio?

Sou desenvolvedor júnior que trabalha para escrever uma atualização para software que recebe dados de uma solução de terceiros, armazena-os em um banco de dados e condiciona os dados para uso por outra solução de terceiros. Nosso software é executado como um serviço do Windows.

Observando o código de uma versão anterior, vejo o seguinte:

        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);

        }

A lógica parece clara: aguarde espaço no conjunto de encadeamentos, verifique se o serviço não foi interrompido, aumente o contador de encadeamentos e enfileire o trabalho. _runningWorkersé decrementado dentro de SomeMethod()uma lockinstrução que chama Monitor.Pulse(_workerLocker).

Minha pergunta é: Existe algum benefício em agrupar todo o código em um único lock, assim:

        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;
                }
            }
        }

Parece que isso pode causar um pouco mais de espera por outros encadeamentos, mas parece que bloquear repetidamente em um único bloco lógico também seria um pouco demorado. No entanto, eu sou novo no multi-threading, então estou assumindo que existem outras preocupações aqui das quais não conheço.

Os únicos outros lugares em que _workerLockerfica bloqueado estão SomeMethod()e apenas com a finalidade de decrementar _runningWorkerse, em seguida, fora do foreachpara aguardar o número de _runningWorkersir para zero antes de registrar e retornar.

Obrigado por qualquer ajuda.

EDIT 8/4/15

Agradecemos a @delnan pela recomendação de usar um semáforo. O código se torna:

        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()é chamado por dentro SomeMethod().

Esta não é uma questão de desempenho. É antes de mais nada uma questão de correção. Se você tiver duas instruções de bloqueio, não poderá garantir atomicidade para operações espalhadas entre eles ou parcialmente fora da instrução de bloqueio. Adaptado para a versão antiga do seu código, isso significa:

Entre o final do while (_runningWorkers >= MaxSimultaneousThreads)e o _runningWorkers++, tudo pode acontecer , porque o código renuncia e adquire o bloqueio no meio. Por exemplo, o encadeamento A pode adquirir o bloqueio pela primeira vez, aguarde até que exista outro encadeamento e saia do loop e do lock. Ele é preemptado e o encadeamento B entra em cena, também aguardando espaço no conjunto de encadeamentos. Porque disse outro segmento sair, lá é sala para que ele não esperar muito tempo em tudo. Agora, o segmento A e o segmento B continuam em alguma ordem, cada um incrementando _runningWorkerse iniciando seu trabalho.

Agora, não há corridas de dados até onde eu possa ver, mas logicamente está errado, pois agora há mais do que MaxSimultaneousThreadstrabalhadores em execução. A verificação é (ocasionalmente) ineficaz porque a tarefa de colocar um slot no pool de threads não é atômica. Isso deve interessar mais do que pequenas otimizações sobre a granularidade do bloqueio! (Observe que, por outro lado, o bloqueio muito cedo ou por muito tempo pode levar facilmente a conflitos).

O segundo trecho corrige esse problema, até onde posso ver. Uma mudança menos invasiva para corrigir o problema pode estar colocando a ++_runningWorkersdireita após a whileaparência, dentro da primeira instrução de bloqueio.

Agora, correção à parte, e quanto ao desempenho? Isso é difícil de dizer. Geralmente, o bloqueio por mais tempo ("grosseiramente") inibe a simultaneidade, mas, como você diz, isso precisa ser equilibrado em relação à sobrecarga da sincronização adicional do bloqueio de baixa granularidade. Geralmente, a única solução é fazer comparações e estar ciente de que existem mais opções do que "bloquear tudo em qualquer lugar" e "bloquear apenas o mínimo necessário". Há uma variedade de padrões e primitivas de simultaneidade e estruturas de dados seguras para encadeamento disponíveis. Por exemplo, parece que os mesmos semáforos de aplicativos foram inventados; portanto, considere usar um deles em vez deste contador de trava manual enrolado à mão.

IMHO, você está fazendo a pergunta errada - você não deve se importar tanto com as compensações da eficiência, mas mais com a correção.

A primeira variante garante que _runningWorkerssó seja acessada durante um bloqueio, mas perde o caso em que _runningWorkerspode ser alterada por outro encadeamento no espaço entre o primeiro bloqueio e o segundo. Honestamente, o código me parece que alguém bloqueou cegamente todos os pontos de acesso _runningWorkerssem pensar nas implicações e nos erros em potencial. Talvez o autor tenha receios supersticiosos sobre a execução da breakdeclaração dentro do lockbloco, mas quem sabe?

Portanto, você deve realmente usar a segunda variante, não porque seja mais ou menos eficiente, mas porque (espero) seja mais correta que a primeira.

As outras respostas são bastante boas e abordam claramente as preocupações de correção. Deixe-me abordar sua pergunta mais geral:

Quanto trabalho devo colocar dentro de uma declaração de bloqueio?

Vamos começar com o conselho padrão que você alude e delnan alude no parágrafo final da resposta aceita:

• Faça o mínimo de trabalho possível ao bloquear um objeto específico. Os bloqueios mantidos por um longo período estão sujeitos a contenção, e a contenção é lenta. Observe que isso implica que a quantidade total de código em um bloqueio específico e a quantidade total de código em todas as instruções de bloqueio que são bloqueadas no mesmo objeto são relevantes.

• Tenha o mínimo de bloqueios possível, para diminuir a probabilidade de bloqueios (ou bloqueios).

O leitor inteligente notará que estes são opostos. O primeiro ponto sugere dividir grandes bloqueios em muitos bloqueios menores e mais refinados para evitar contendas. O segundo sugere a consolidação de bloqueios distintos no mesmo objeto de bloqueio para evitar conflitos.

O que podemos concluir do fato de que o melhor conselho padrão é completamente contraditório? Na verdade, temos bons conselhos:

• Não vá lá em primeiro lugar. Se você está compartilhando memória entre threads, está se abrindo para um mundo de dor.

Meu conselho é que, se você deseja simultaneidade, use processos como sua unidade de simultaneidade. Se você não pode usar processos, use domínios de aplicativo. Se você não puder usar domínios de aplicativo, tenha seus threads gerenciados pela Task Parallel Library e escreva seu código em termos de tarefas de alto nível (tarefas), em vez de threads de baixo nível (trabalhadores).

Se você absolutamente absolutamente deve usar primitivas de simultaneidade de baixo nível, como threads ou semáforos, use-as para criar uma abstração de nível superior que capture o que você realmente precisa. Você provavelmente descobrirá que a abstração de nível superior é algo como "executar uma tarefa de forma assíncrona que pode ser cancelada pelo usuário"; Você provavelmente descobrirá que precisa de algo como inicialização lenta e segura do thread; não use seu próprio uso Lazy<T>, escrito por especialistas. Use coleções thread-safe (imutáveis ​​ou não) escritas por especialistas. Mova o nível de abstração o mais alto possível.