Ile pracy powinienem umieścić w instrukcji blokady?

Jestem młodszym programistą pracującym nad pisaniem aktualizacji oprogramowania, które odbiera dane z rozwiązania innej firmy, przechowuje je w bazie danych, a następnie warunkuje dane do wykorzystania przez inne rozwiązanie innej firmy. Nasze oprogramowanie działa jako usługa systemu Windows.

Patrząc na kod z poprzedniej wersji, widzę to:

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

        }

Logika wydaje się jasna: poczekaj na miejsce w puli wątków, upewnij się, że usługa nie została zatrzymana, a następnie zwiększ licznik wątków i ustaw kolejkę w pracy. _runningWorkersjest zmniejszana wewnątrz SomeMethod()wewnątrz lockoświadczenie, że wywołuje Monitor.Pulse(_workerLocker).

Moje pytanie brzmi: czy jest jakaś korzyść z grupowania całego kodu w jednym lock, tak jak to:

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

Wygląda na to, że może to wymagać nieco więcej czasu na inne wątki, ale wydaje się, że wielokrotne blokowanie w jednym bloku logicznym byłoby również trochę czasochłonne. Jednak jestem nowy w wielowątkowości, więc zakładam, że istnieją inne obawy, których nie jestem świadomy.

Jedyne inne miejsca, w których _workerLockerzostanie zablokowany SomeMethod(), znajdują się tylko w celu zmniejszenia _runningWorkers, a następnie na zewnątrz, foreachaby poczekać, aż liczba _runningWorkersspadnie do zera przed zalogowaniem i powrotem.

Dziękuję za wszelką pomoc.

EDYCJA 4/8/15

Dzięki @delnan za zalecenie użycia semafora. Kod staje się:

        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()nazywa się wewnątrz SomeMethod().

To nie jest kwestia wydajności. Jest to przede wszystkim kwestia poprawności. Jeśli masz dwie instrukcje blokady, nie możesz zagwarantować atomowości operacji, które są między nimi rozłożone lub częściowo poza instrukcją blokady. Dostosowane do starej wersji Twojego kodu, oznacza to:

Między końcem while (_runningWorkers >= MaxSimultaneousThreads)a _runningWorkers++, w ogóle cokolwiek może się zdarzyć , bo zrzeknie kod i ponownie nabywa blokadę pomiędzy. Na przykład wątek A może uzyskać blokadę po raz pierwszy, poczekaj, aż wyjdzie jakiś inny wątek, a następnie wyrwij się z pętli i lock. Następnie jest zapobiegane, a wątek B wchodzi do obrazu, czekając również na miejsce w puli wątków. Ponieważ wspomniany drugi wątek zakończył pracę, jest miejsce, więc nie trzeba długo czekać. Zarówno wątek A, jak i wątek B przechodzą teraz w pewnej kolejności, każdy zwiększa _runningWorkersi rozpoczyna pracę.

Teraz, jak widzę, nie ma wyścigów danych, ale logicznie jest to złe, ponieważ działa teraz więcej niż MaxSimultaneousThreadspracownicy. Kontrola jest (czasami) nieskuteczna, ponieważ zadanie polegające na przyjęciu szczeliny w puli wątków nie jest atomowe. Powinno to dotyczyć więcej niż drobnych optymalizacji granularności zamka! (Uwaga: odwrotnie, zbyt wczesne lub zbyt długie blokowanie może łatwo doprowadzić do impasu).

Drugi fragment rozwiązuje ten problem, o ile widzę. Mniej inwazyjną zmianą w celu rozwiązania problemu może być umieszczenie ++_runningWorkerszaraz po whilewyglądzie wewnątrz pierwszej instrukcji blokady.

Pomijając poprawność, a co z wydajnością? Trudno powiedzieć. Zasadniczo blokowanie na dłuższy czas („zgrubnie”) hamuje współbieżność, ale, jak mówisz, musi to być zrównoważone narzutem z dodatkowej synchronizacji blokowania drobnoziarnistego. Zasadniczo jedynym rozwiązaniem jest analiza porównawcza i świadomość, że istnieje więcej opcji niż „zablokuj wszystko wszędzie” i „zablokuj tylko absolutne minimum”. Dostępnych jest wiele wzorów i operacji podstawowych i współbieżnych struktur danych. Na przykład wydaje się, że właśnie te semafory zostały wymyślone, więc zastanów się nad użyciem jednego z nich zamiast tego ręcznie zwijanego ręcznie blokowanego licznika.

IMHO zadajesz złe pytanie - nie powinieneś tak bardzo dbać o kompromisy wydajnościowe, ale bardziej o poprawność.

Pierwszy wariant zapewnia _runningWorkersdostęp tylko podczas blokady, ale pomija przypadek, w którym _runningWorkersmoże zostać zmieniony przez inny wątek w szczelinie między pierwszą blokadą a drugą. Szczerze mówiąc, kod sprawdza, czy ktoś ślepo blokuje wszystkie punkty dostępu, _runningWorkersnie zastanawiając się nad implikacjami i potencjalnymi błędami. Może autor miał jakieś przesądne obawy przed wykonaniem breakinstrukcji wewnątrz lockbloku, ale kto wie?

Powinieneś więc właściwie użyć drugiego wariantu, nie dlatego, że jest mniej lub bardziej wydajny, ale dlatego, że (mam nadzieję) jest bardziej poprawny niż pierwszy.

Pozostałe odpowiedzi są dość dobre i wyraźnie dotyczą problemów z poprawnością. Pozwól, że odpowiem na twoje bardziej ogólne pytanie:

Ile pracy powinienem umieścić w instrukcji blokady?

Zacznijmy od standardowej porady, do której nawiązujesz i do której nawiązujesz w ostatnim akapicie przyjętej odpowiedzi:

• Wykonuj jak najmniej pracy przy blokowaniu określonego obiektu. Zamki trzymane przez długi czas podlegają rywalizacji, a rywalizacja jest powolna. Zauważ, że oznacza to, że łączna ilość kodu w konkretnej blokadzie i całkowita ilość kodu we wszystkich instrukcjach blokujących blokujących ten sam obiekt są istotne.

• Miej jak najmniej zamków, aby zmniejszyć prawdopodobieństwo zakleszczenia (lub blokady żywej).

Sprytny czytelnik zauważy, że są to przeciwieństwa. Pierwszy punkt sugeruje rozbicie dużych zamków na wiele mniejszych, drobnoziarnistych zamków, aby uniknąć rywalizacji. Drugi sugeruje konsolidację odrębnych blokad w ten sam obiekt blokady, aby uniknąć zakleszczeń.

Co możemy wyciągnąć z faktu, że najlepsza standardowa rada jest całkowicie sprzeczna? Docieramy do naprawdę dobrych rad:

• Nie idź tam w pierwszej kolejności. Jeśli dzielisz pamięć między wątkami, otwierasz się na świat bólu.

Moja rada jest taka: jeśli chcesz współbieżności, użyj procesów jako jednostki współbieżności. Jeśli nie możesz użyć procesów, użyj domen aplikacji. Jeśli nie możesz używać domen aplikacji, poproś o zarządzanie wątkami w Bibliotece zadań równoległych i napisz kod w kategoriach zadań wysokiego poziomu (zadań) zamiast wątków niskiego poziomu (pracowników).

Jeśli absolutnie pozytywnie musisz użyć prymitywów współbieżności niskiego poziomu, takich jak wątki lub semafory, użyj ich, aby zbudować abstrakcję wyższego poziomu, która przechwytuje to, czego naprawdę potrzebujesz. Prawdopodobnie przekonasz się, że abstrakcja wyższego poziomu przypomina coś takiego: „wykonaj zadanie asynchronicznie, które może zostać anulowane przez użytkownika”, i hej, TPL już to obsługuje, więc nie musisz rzucać własnym. Prawdopodobnie okaże się, że potrzebujesz czegoś takiego jak wątkowa inicjalizacja leniwa; nie rzucaj własnym, użyj Lazy<T>, który został napisany przez ekspertów. Używaj bezpiecznych kolekcji wątków (niezmiennych lub innych) napisanych przez ekspertów. Podnieś poziom abstrakcji w górę jak najwyżej.