Quelle quantité de travail dois-je placer dans une instruction de verrouillage?

Je suis un développeur junior travaillant sur l'écriture d'une mise à jour pour un logiciel qui reçoit des données d'une solution tierce, les stocke dans une base de données, puis conditionne les données pour une utilisation par une autre solution tierce. Notre logiciel fonctionne comme un service Windows.

En regardant le code d'une version précédente, je vois ceci:

        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 logique semble claire: attendez de la place dans le pool de threads, assurez-vous que le service n'a pas été arrêté, puis incrémentez le compteur de threads et mettez le travail en file d'attente. _runningWorkersest décrémenté à l' SomeMethod()intérieur d'une lockinstruction qui appelle ensuite Monitor.Pulse(_workerLocker).

Ma question est: Y a - t-il un avantage à regrouper tout le code dans un seul lock, comme ceci:

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

Il semble que cela puisse causer un peu plus d'attente pour d'autres threads, mais il semble alors que le verrouillage à plusieurs reprises dans un seul bloc logique prendrait également un peu de temps. Cependant, je suis nouveau dans le multi-threading, donc je suppose qu'il y a d'autres problèmes ici que je ne connais pas.

Le seul autre endroit où _workerLockerest verrouillé est à l' intérieur SomeMethod(), et uniquement dans le but de décrémenter _runningWorkers, puis à l'extérieur de l' foreachattente du nombre de _runningWorkerszéro pour se connecter et revenir.

Merci pour toute aide.

EDIT 4/8/15

Merci à @delnan pour la recommandation d'utiliser un sémaphore. Le code devient:

        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()est appelé à l'intérieur SomeMethod().

Ce n'est pas une question de performance. C'est d'abord et avant tout une question de rectitude. Si vous avez deux instructions de verrouillage, vous ne pouvez pas garantir l'atomicité pour les opérations réparties entre elles, ou partiellement en dehors de l'instruction de verrouillage. Adapté à l'ancienne version de votre code, cela signifie:

Entre la fin du while (_runningWorkers >= MaxSimultaneousThreads)et le _runningWorkers++, tout peut arriver , car le code se rend et acquiert le verrou entre les deux. Par exemple, le thread A peut acquérir le verrou pour la première fois, attendre jusqu'à ce qu'un autre thread se termine, puis sortir de la boucle et du lock. Il est alors préempté et le thread B entre dans l'image, en attendant également de la place dans le pool de threads. Parce que ledit autre thread a quitté, il y a de la place donc il n'attend pas du tout très longtemps. Les threads A et B se poursuivent maintenant dans un certain ordre, chacun s'incrémentant _runningWorkerset commençant leur travail.

Maintenant, il n'y a pas de course aux données pour autant que je puisse voir, mais logiquement c'est faux, car il y a maintenant plus que des MaxSimultaneousThreadstravailleurs en cours d'exécution. La vérification est (parfois) inefficace car la tâche de prendre un emplacement dans le pool de threads n'est pas atomique. Cela devrait vous concerner plus que de petites optimisations autour de la granularité des verrous! (Notez qu'à l'inverse, un verrouillage trop tôt ou trop long peut facilement entraîner des blocages.)

Le deuxième extrait de code corrige ce problème, pour autant que je puisse voir. Un changement moins invasif pour résoudre le problème pourrait être de mettre le ++_runningWorkersdroit après le whilelook, à l'intérieur de la première instruction de verrouillage.

Maintenant, la correction mise à part, qu'en est-il des performances? C'est difficile à dire. Généralement, le verrouillage pendant une période plus longue ("grossièrement") empêche la concurrence, mais comme vous le dites, cela doit être mis en balance avec les frais généraux de la synchronisation supplémentaire du verrouillage à grain fin. En règle générale, la seule solution est l'analyse comparative et le fait de savoir qu'il existe plus d'options que «tout verrouiller partout» et «verrouiller uniquement le strict minimum». Il existe une multitude de modèles et de primitives de concurrence et de structures de données thread-safe. Par exemple, cela semble que les sémaphores de l'application même aient été inventés, alors pensez à en utiliser un au lieu de ce compteur verrouillé à la main.

À mon humble avis, vous posez la mauvaise question - vous ne devriez pas vous soucier autant des compromis d'efficacité, mais plutôt de la correction.

La première variante s'assure _runningWorkersque l'accès n'est possible que pendant un verrou, mais il manque le cas où il _runningWorkerspourrait être modifié par un autre thread dans l'espace entre le premier verrou et le second. Honnêtement, le code me semble si quelqu'un a mis des verrous aveugles autour de tous les points d'accès _runningWorkerssans penser aux implications et aux erreurs potentielles. Peut-être que l'auteur avait des craintes superstitieuses concernant l'exécution de la breakdéclaration à l'intérieur du lockbloc, mais qui sait?

Donc, vous devriez réellement utiliser la deuxième variante, non pas parce qu'elle est plus ou moins efficace, mais parce que (espérons-le) plus correcte que la première.

Les autres réponses sont assez bonnes et répondent clairement aux problèmes d'exactitude. Permettez-moi de répondre à votre question plus générale:

Quelle quantité de travail dois-je placer dans une instruction de verrouillage?

Commençons par le conseil standard auquel vous faites allusion et auquel vous faites allusion dans le dernier paragraphe de la réponse acceptée:

• Faites le moins de travail possible en verrouillant un objet particulier. Les verrous qui sont maintenus pendant une longue période sont sujets à contention et la contention est lente. Notez que cela implique que la quantité totale de code dans un verrou particulier et la quantité totale de code dans toutes les instructions de verrouillage qui se verrouillent sur le même objet sont toutes deux pertinentes.

• Avoir le moins de verrous possible, pour réduire la probabilité de blocages (ou de verrous).

Le lecteur intelligent remarquera qu'il s'agit d'opposés. Le premier point suggère de diviser les gros verrous en de nombreux verrous plus petits et plus fins pour éviter les conflits. Le second suggère de consolider des verrous distincts dans le même objet de verrouillage pour éviter les blocages.

Que pouvons-nous conclure du fait que les meilleurs conseils standard sont totalement contradictoires? Nous obtenons en fait de bons conseils:

• N'y allez pas en premier lieu. Si vous partagez la mémoire entre les fils, vous vous ouvrez à un monde de douleur.

Mon conseil est, si vous voulez la concurrence, utilisez des processus comme unité de concurrence. Si vous ne pouvez pas utiliser de processus, utilisez des domaines d'application. Si vous ne pouvez pas utiliser de domaines d'application, faites gérer vos threads par la bibliothèque parallèle de tâches et écrivez votre code en termes de tâches de haut niveau (travaux) plutôt qu'en termes de threads de bas niveau (travailleurs).

Si vous devez absolument utiliser des primitives de concurrence de bas niveau comme des threads ou des sémaphores, utilisez-les pour créer une abstraction de niveau supérieur qui capture ce dont vous avez vraiment besoin. Vous constaterez probablement que l'abstraction de niveau supérieur est quelque chose comme "effectuer une tâche de manière asynchrone qui peut être annulée par l'utilisateur", et bon, le TPL le prend déjà en charge, vous n'avez donc pas besoin de lancer la vôtre. Vous constaterez probablement que vous avez besoin de quelque chose comme une initialisation paresseuse sécurisée pour les threads; ne roulez pas le vôtre, utilisez Lazy<T>, qui a été écrit par des experts. Utilisez des collections threadsafe (immuables ou non) écrites par des experts. Augmentez le niveau d'abstraction aussi haut que possible.