Wie viel Arbeit sollte ich in eine lock-Anweisung stecken?

Ich bin ein Junior-Entwickler, der an der Erstellung eines Updates für Software arbeitet, die Daten von einer Drittanbieterlösung empfängt, diese in einer Datenbank speichert und dann die Daten für die Verwendung durch eine andere Drittanbieterlösung aufbereitet. Unsere Software wird als Windows-Dienst ausgeführt.

Wenn ich mir den Code einer früheren Version ansehe, sehe ich Folgendes:

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

        }

Die Logik scheint klar zu sein: Warten Sie auf Platz im Thread-Pool, stellen Sie sicher, dass der Dienst nicht gestoppt wurde, erhöhen Sie dann den Thread-Zähler und stellen Sie die Arbeit in eine Warteschlange. _runningWorkerswird SomeMethod()innerhalb einer lockAnweisung dekrementiert , die dann aufruft Monitor.Pulse(_workerLocker).

Meine Frage ist: Gibt es irgendeinen Vorteil, wenn man den gesamten Code in einer einzigen gruppiert lock, wie folgt :

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

Es scheint, als würde ein bisschen mehr auf andere Threads gewartet, aber dann scheint das wiederholte Sperren in einem einzelnen logischen Block auch etwas zeitaufwändig zu sein. Ich bin jedoch neu in Multithreading und gehe daher davon aus, dass es hier andere Bedenken gibt, die mir nicht bewusst sind.

Die einzigen anderen Orte, an denen _workerLockergesperrt wird, befinden sich in SomeMethod()und nur zum Zweck des Dekrementierens _runningWorkers, und dann außerhalb der foreach, um zu warten, bis die Anzahl _runningWorkersNull erreicht ist, bevor Sie sich anmelden und zurückkehren.

Vielen Dank für jede Hilfe.

EDIT 4/8/15

Vielen Dank an @delnan für die Empfehlung, ein Semaphor zu verwenden. Der Code wird:

        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()heißt drinnen SomeMethod().

Dies ist keine Frage der Leistung. Es ist in erster Linie eine Frage der Richtigkeit. Wenn Sie über zwei lock-Anweisungen verfügen, können Sie für Operationen, die zwischen ihnen oder teilweise außerhalb der lock-Anweisung verteilt sind, keine Atomizität garantieren. Zugeschnitten auf die alte Version Ihres Codes bedeutet dies:

Zwischen dem Ende des while (_runningWorkers >= MaxSimultaneousThreads)und des _runningWorkers++kann alles passieren , weil der Code die Sperre dazwischen aufgibt und wieder erlangt. Beispiel: Thread A kann die Sperre zum ersten Mal aktivieren, warten, bis ein anderer Thread beendet ist, und dann die Schleife und die Schleife verlassen lock. Es wird dann vorgezogen, und Thread B tritt in das Bild ein und wartet ebenfalls auf Platz im Thread-Pool. Weil der andere Thread aufgehört hat, ist Platz, so dass er gar nicht so lange wartet. Sowohl Thread A als auch Thread B werden nun in einer bestimmten Reihenfolge fortgesetzt, wobei jeder Thread inkrementiert _runningWorkersund mit seiner Arbeit begonnen wird.

Soweit ich sehen kann , gibt es jetzt keine Datenrennen, aber logischerweise ist es falsch, da jetzt mehr als MaxSimultaneousThreadsArbeiter am Laufen sind. Die Überprüfung ist (gelegentlich) ineffektiv, da die Aufgabe, einen Slot im Thread-Pool zu belegen, nicht atomar ist. Dies sollte Sie mehr als nur kleine Optimierungen in Bezug auf die Sperrgranularität betreffen! (Beachten Sie, dass ein zu frühes oder zu langes Sperren leicht zu Deadlocks führen kann.)

Das zweite Snippet behebt dieses Problem, soweit ich sehen kann. Eine weniger invasive Änderung zur Behebung des Problems könnte darin bestehen, die erste lock-Anweisung ++_runningWorkersnach dem whileLook zu setzen .

Abgesehen von der Korrektheit, was ist mit der Leistung? Das ist schwer zu sagen. Im Allgemeinen verhindert das Sperren über einen längeren Zeitraum ("grob") die Parallelität, aber wie Sie sagen, muss dies gegen den Overhead aus der zusätzlichen Synchronisierung der feinkörnigen Sperrung abgewogen werden. Im Allgemeinen besteht die einzige Lösung darin, ein Benchmarking durchzuführen und sich darüber im Klaren zu sein, dass es mehr Optionen gibt als "Alles überall sperren" und "Nur das Nötigste sperren". Es gibt eine Fülle von Mustern und Nebenläufigkeitsprimitiven und thread-sicheren Datenstrukturen. Dies scheint zum Beispiel genau für die Anwendungssemaphore erfunden worden zu sein. Verwenden Sie also einen dieser Semaphore anstelle dieses handgerollten handgesperrten Zählers.

IMHO stellen Sie die falsche Frage - Sie sollten sich nicht so sehr für Effizienzkompromisse interessieren, sondern mehr für Korrektheit.

Die erste Variante stellt sicher, dass _runningWorkersnur während einer Sperre zugegriffen wird. Sie übersieht jedoch den Fall, _runningWorkersdass ein anderer Thread in der Lücke zwischen der ersten und der zweiten Sperre möglicherweise Änderungen vornimmt . Ehrlich gesagt, der Code sieht für mich so aus, als hätte jemand blind alle Zugangspunkte gesperrt, _runningWorkersohne über die Auswirkungen und die möglichen Fehler nachzudenken. Vielleicht hatte der Autor einige abergläubische Befürchtungen, die breakAussage innerhalb des lockBlocks auszuführen , aber wer weiß?

Sie sollten also die zweite Variante verwenden, nicht weil sie mehr oder weniger effizient ist, sondern weil sie (hoffentlich) korrekter ist als die erste.

Die anderen Antworten sind recht gut und gehen eindeutig auf die Korrektheitsprobleme ein. Lassen Sie mich auf Ihre allgemeinere Frage eingehen:

Wie viel Arbeit sollte ich in eine lock-Anweisung stecken?

Beginnen wir mit den Standardempfehlungen, auf die Sie anspielen und auf die Delnan im letzten Absatz der akzeptierten Antwort anspielt:

• Arbeiten Sie so wenig wie möglich, während Sie ein bestimmtes Objekt verriegeln. Locks, die über einen langen Zeitraum gehalten werden, sind Gegenstand von Konflikten, und der Konflikt ist langsam. Beachten Sie, dass dies bedeutet , dass die Gesamtcodemenge in einem bestimmten Schloss und die Gesamtmenge des Codes in allen Lock - Anweisungen , die auf dem gleichen Objekt sperren sind beide relevant.

• Sorgen Sie für möglichst wenige Sperren, um die Wahrscheinlichkeit von Deadlocks (oder Livelocks) zu verringern.

Der kluge Leser wird feststellen, dass dies Gegensätze sind. Der erste Punkt schlägt vor, große Sperren in viele kleinere, feinkörnigere Sperren aufzuteilen, um Konflikte zu vermeiden. Im zweiten Schritt wird vorgeschlagen, unterschiedliche Sperren in demselben Sperrobjekt zu konsolidieren, um Deadlocks zu vermeiden.

Was können wir daraus schließen, dass die beste Standardberatung durchaus widersprüchlich ist? Wir bekommen dazu eigentlich gute Ratschläge:

• Gehen Sie nicht in erster Linie dorthin. Wenn Sie die Erinnerung zwischen Threads teilen, öffnen Sie sich für eine Welt voller Schmerzen.

Mein Rat ist, wenn Sie Parallelität möchten, verwenden Sie Prozesse als Einheit der Parallelität. Wenn Sie keine Prozesse verwenden können, verwenden Sie Anwendungsdomänen. Wenn Sie keine Anwendungsdomänen verwenden können, lassen Sie Ihre Threads von der Task Parallel Library verwalten und schreiben Sie Ihren Code in Form von übergeordneten Tasks (Jobs) anstelle von übergeordneten Threads (Workers).

Wenn Sie unbedingt Nebenläufigkeiten auf niedriger Ebene wie Threads oder Semaphoren verwenden müssen, können Sie mit ihnen eine übergeordnete Abstraktion erstellen , die erfasst, was Sie wirklich benötigen. Sie werden wahrscheinlich feststellen, dass die übergeordnete Abstraktion so etwas wie "asynchrones Ausführen einer Aufgabe, die vom Benutzer abgebrochen werden kann" ist, und die TPL unterstützt dies bereits, sodass Sie keine eigene Aufgabe ausführen müssen. Sie werden wahrscheinlich feststellen, dass Sie so etwas wie eine thread-sichere verzögerte Initialisierung benötigen. Würfle nicht deine eigenen, benutze Lazy<T>die von Experten geschrieben wurden. Verwenden Sie Thread-sichere Sammlungen (unveränderlich oder anderweitig), die von Experten geschrieben wurden. Stellen Sie die Abstraktionsebene so hoch wie möglich ein.