2013-01-24 30 views
6

私はParallel.ForEachパターンを実装しようとしており、進行状況を追跡しようとしていますが、ロックに関しては何か不足しています。次の例では、threadCount = 1の場合は1000にカウントされますが、threadCount> 1の場合はカウントされません。これを行う正しい方法は何ですか。適切なParallel.ForEachのロックと進捗状況の報告

class Program 
{ 
    static void Main() 
    { 
     var progress = new Progress(); 
     var ids = Enumerable.Range(1, 10000); 
     var threadCount = 2; 

     Parallel.ForEach(ids, new ParallelOptions { MaxDegreeOfParallelism = threadCount }, id => { progress.CurrentCount++; }); 

     Console.WriteLine("Threads: {0}, Count: {1}", threadCount, progress.CurrentCount); 
     Console.ReadKey(); 
    } 
} 

internal class Progress 
{ 
    private Object _lock = new Object(); 
    private int _currentCount; 
    public int CurrentCount 
    { 
     get 
     { 
     lock (_lock) 
     { 
      return _currentCount; 
     } 
     } 
     set 
     { 
     lock (_lock) 
     { 
      _currentCount = value; 
     } 
     } 
    } 
} 

答えて

16

count変数を共有する)複数のスレッドからcount++のようなものを呼び出すと、通常の問題は、この一連のイベントが発生する可能性があることです。

  1. スレッドAがcountの値を読み取ります。
  2. スレッドBは、countの値を読み取ります。
  3. スレッドAはローカルコピーをインクリメントします。
  4. スレッドBはローカルコピーをインクリメントします。
  5. スレッドAは、インクリメントされた値をcountに書き戻します。
  6. スレッドBは、インクリメントされた値をcountに書き戻します。

このように、スレッドAによって書き込まれた値はスレッドBによって上書きされるため、値は実際には1回だけインクリメントされます。

コードでは、操作1、2(get)および5,6(set)のロックを追加しますが、問題のあるイベントシーケンスを防止するために何もしません。

lock (progressLock) 
{ 
    progress.CurrentCount++; 
} 

をあなたが唯一のことがわかっている場合:スレッドAが値をインクリメントしている間、スレッドBがまったくアクセスできないように、あなたがする必要がどのような

は、全体の動作をロックすることですインクリメントが必要な場合は、これをカプセル化するメソッドProgressを作成することができます。

+0

ありがとう、まさに何が起きているのですか? Progressの追加メソッドを追加して、ロックを適用してカウンタを増やしました。 –

+0

変数progressLockは "進捗状況"になるはずですか? – eschneider

0

性質からロック文を削除し、本体を変更します。

object sync = new object(); 
     Parallel.ForEach(ids, new ParallelOptions {MaxDegreeOfParallelism = threadCount}, 
         id => 
          { 
           lock(sync) 
           progress.CurrentCount++; 
          }); 
+0

理由を説明できますか? – svick

+0

私は、ループ本体のロックステートメントは原子的だと思います。プロパティでLockステートメントは分離されています。これは、プロパティでメソッドがコンパイルされるためです。 – chameleon86

+0

インクリメント演算子にはgetterとsetterの呼び出しが含まれているため、x ++はx = x + 1の略語です。したがって、値が読み込まれて更新される前にロックされます。しかし、私はまだフィールドが良いと思う。 :) –

0

ここでの問題は++がアトミックではないということです - 1つのスレッドが読み、インクリメント値を読み取る別のスレッド間の値を、それすることができますインクリメントされた値を格納します。これはおそらく、あなたのintをラップするプロパティが存在するという事実によって複合化されます。

themsevesの順序が呼び出されるlockブロックを停止するには何もないとセッターとゲッターの周り

Thread 1  Thread 2 
reads 5   . 
.    reads 5 
.    writes 6 
writes 6!  . 

ロックは、このことを助けていません。

通常、私はInterlocked.Incrementを使用することをお勧めしますが、これはプロパティでは使用できません。

代わりに_lockを公開し、lockブロックをprogress.CurrentCount++;コールの周りに置くことができます。

+0

私はロックを公開するのは悪い考えだと思います。だれかがそれを使用することができ、簡単にデッドロックにつながる可能性があるからです。私はもっ​​と良い解決策は、メイン() 'で直接ロックを持つことだと思う。 – svick

+0

'int'を使って何か他のことをしているコードがあれば、そのコードがすべてのオブジェクトの間で別々のロックオブジェクトを共有しようとするのではなく、同じオブジェクトをロックする方が簡単です。 – Rawling

1

最も簡単な解決策は、実際のフィールドでプロパティを置換することであっただろう、と

lock { ++progress.CurrentCount; } 

は(私は個人として、ポストインクリメント以上前置インクリメントの外観を好む「++。」の事の衝突フィールドの更新はフィールドを更新するメソッドを呼び出すよりも速いため、オーバーヘッドと競合を減らすという追加の利点があります。

もちろん、プロパティとしてカプセル化することも利点があります。 IMOは、フィールドとプロパティの構文が同一であるため、プロパティが自動実装されているか同等の場合に、フィールド上のプロパティを使用する利点は、依存関係を構築して展開することなく1つのアセンブリを展開するシナリオがある場合のみです新しいアセンブリ。それ以外の場合は、より高速なフィールドを使用することもできます。値をチェックしたり、副作用を追加する必要が生じた場合は、フィールドをプロパティに変換して再構築するだけです。したがって、実際の多くの場合、フィールドを使用する場合にはペナルティはありません。

しかし、多くの開発チームが独断で動いていて、StyleCopのようなツールを使用して独り向きを執行しています。そのようなツールは、コーダーとは異なり、フィールドを使用できるかどうかを判断するのに十分スマートではないため、常に「StyleCopでもチェックするだけの単純なルール」は「フィールドをプロパティとしてカプセル化」、「パブリックフィールドを使用しない」、等...

15

古い質問ですが、より良い答えがあると思います。

Interlocked.Increment(ref progress)を使用して進行状況を報告することができます。これにより、書き込み操作を進行状況にロックする心配がありません。

0

データベースまたはファイルシステムの操作をロックする代わりに、ローカルのバッファ変数に格納する方が適切です。ロックによりパフォーマンスが低下します。

関連する問題