¿Cuánto trabajo debo colocar dentro de una declaración de bloqueo?

Soy un desarrollador junior que trabaja en la redacción de una actualización para el software que recibe datos de una solución de terceros, los almacena en una base de datos y luego condiciona los datos para su uso por otra solución de terceros. Nuestro software se ejecuta como un servicio de Windows.

Mirando el código de una versión anterior, veo esto:

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

        }

La lógica parece clara: espere espacio en el grupo de subprocesos, asegúrese de que el servicio no se haya detenido, luego incremente el contador de subprocesos y ponga en cola el trabajo. _runningWorkersse decrementa dentro de SomeMethod()una lockdeclaración que luego llama Monitor.Pulse(_workerLocker).

Mi pregunta es: ¿hay algún beneficio en agrupar todo el código dentro de un único lock, como este:

        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 puede causar un poco más de espera para otros subprocesos, pero luego parece que bloquear repetidamente en un solo bloque lógico también requeriría algo de tiempo. Sin embargo, soy nuevo en subprocesos múltiples, por lo que supongo que hay otras preocupaciones aquí que desconozco.

Los únicos otros lugares donde _workerLockerse bloquea es adentro SomeMethod(), y solo con el propósito de disminuir _runningWorkers, y luego afuera foreachpara esperar a que el número _runningWorkersllegue a cero antes de iniciar sesión y regresar.

Gracias por cualquier ayuda.

EDITAR 4/8/15

Gracias a @delnan por la recomendación de usar un semáforo. El código se convierte en:

        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()se llama adentro SomeMethod().

Esta no es una cuestión de rendimiento. Es ante todo una cuestión de corrección. Si tiene dos declaraciones de bloqueo, no puede garantizar la atomicidad para las operaciones que se distribuyen entre ellas o parcialmente fuera de la declaración de bloqueo. Diseñado para la versión anterior de su código, esto significa:

Entre el final de while (_runningWorkers >= MaxSimultaneousThreads)y el _runningWorkers++, puede pasar cualquier cosa , porque el código se rinde y vuelve a adquirir el bloqueo en el medio. Por ejemplo, el subproceso A puede adquirir el bloqueo por primera vez, esperar hasta que salga algún otro subproceso y luego salir del bucle y el lock. Luego se adelanta, y el hilo B entra en la imagen, también esperando espacio en el grupo de hilos. Debido a que dicho otro hilo dejar de fumar, no es habitación, así que no espera mucho tiempo en absoluto. Tanto el subproceso A como el subproceso B ahora continúan en algún orden, cada uno incrementando _runningWorkersy comenzando su trabajo.

Ahora, hasta donde puedo ver, no hay carreras de datos, pero lógicamente está mal, ya que ahora hay más que MaxSimultaneousThreadstrabajadores corriendo. La verificación es (ocasionalmente) ineficaz porque la tarea de tomar una ranura en el grupo de subprocesos no es atómica. ¡Esto debería preocuparte más que pequeñas optimizaciones en torno a la granularidad de bloqueo! (Tenga en cuenta que, por el contrario, bloquear demasiado pronto o durante demasiado tiempo puede conducir fácilmente a puntos muertos).

El segundo fragmento soluciona este problema, por lo que puedo ver. Un cambio menos invasivo para solucionar el problema podría ser colocarlo ++_runningWorkersjusto después del whileaspecto, dentro de la primera declaración de bloqueo.

Ahora, aparte de la corrección, ¿qué pasa con el rendimiento? Esto es difícil de decir. En general, el bloqueo durante un tiempo más prolongado ("grueso") inhibe la concurrencia, pero como usted dice, esto debe equilibrarse con la sobrecarga de la sincronización adicional del bloqueo de grano fino. En general, la única solución es la evaluación comparativa y tener en cuenta que hay más opciones que "bloquear todo en todas partes" y "bloquear solo el mínimo". Hay una gran cantidad de patrones y primitivas de concurrencia y estructuras de datos seguras para subprocesos disponibles. Por ejemplo, esto parece que los semáforos de la misma aplicación fueron inventados, así que considere usar uno de esos en lugar de este contador enrollado a mano.

En mi humilde opinión, está haciendo la pregunta equivocada: no debe preocuparse tanto por las compensaciones de eficiencia, sino más bien por la corrección.

La primera variante asegura que _runningWorkerssolo se acceda durante un bloqueo, pero se pierde el caso en el que _runningWorkerspodría ser cambiado por otro hilo en el espacio entre el primer bloqueo y el segundo. Honestamente, el código me mira si alguien ha bloqueado ciegamente todos los puntos de acceso _runningWorkerssin pensar en las implicaciones y los posibles errores. Tal vez el autor tenía algunos temores supersticiosos sobre la ejecución de la breakdeclaración dentro del lockbloque, pero ¿quién sabe?

Por lo tanto, debería usar la segunda variante, no porque sea más o menos eficiente, sino porque es (con suerte) más correcta que la primera.

Las otras respuestas son bastante buenas y abordan claramente los problemas de corrección. Déjame abordar tu pregunta más general:

¿Cuánto trabajo debo colocar dentro de una declaración de bloqueo?

Comencemos con los consejos estándar, a los que alude y que alnan alude en el párrafo final de la respuesta aceptada:

• Haga el menor trabajo posible mientras bloquea un objeto en particular. Los bloqueos que se mantienen durante mucho tiempo están sujetos a contención y la contención es lenta. Tenga en cuenta que esto implica que la cantidad total de código en un bloqueo particular y la cantidad total de código en todas las declaraciones de bloqueo que se bloquean en el mismo objeto son relevantes.

• Tenga la menor cantidad de bloqueos posible, para reducir la probabilidad de puntos muertos (o bloqueos vivos).

El lector inteligente notará que estos son opuestos. El primer punto sugiere dividir las cerraduras grandes en muchas cerraduras más pequeñas y de grano más fino para evitar la contención. El segundo sugiere consolidar bloqueos distintos en el mismo objeto de bloqueo para evitar puntos muertos.

¿Qué podemos concluir del hecho de que el mejor consejo estándar es completamente contradictorio? Llegamos a buenos consejos:

• No vayas allí en primer lugar. Si está compartiendo memoria entre hilos, se está abriendo a un mundo de dolor.

Mi consejo es que, si desea concurrencia, use los procesos como su unidad de concurrencia. Si no puede utilizar procesos, utilice dominios de aplicación. Si no puede usar dominios de aplicación, haga que sus hilos sean gestionados por la Biblioteca de tareas paralelas y escriba su código en términos de tareas de alto nivel (trabajos) en lugar de hilos de bajo nivel (trabajadores).

Si absolutamente debe usar primitivas de concurrencia de bajo nivel como hilos o semáforos, entonces úselos para construir una abstracción de nivel superior que capture lo que realmente necesita. Probablemente encontrará que la abstracción de nivel superior es algo así como "realizar una tarea de forma asincrónica que puede ser cancelada por el usuario", y bueno, el TPL ya es compatible con eso, por lo que no necesita rodar la suya. Probablemente encontrará que necesita algo como inicialización perezosa segura para subprocesos; no enrolles tu propio uso Lazy<T>, que fue escrito por expertos. Utilice colecciones seguras (inmutables o no) escritas por expertos. Mueva el nivel de abstracción lo más alto posible.