Where is the correct location to use lock

Question

I try to find the best similarity in multithreaded environment.

Is there any better alternative or both versions are same below?

 // float bestSimilarity is shared
 // float _similarity is local

 lock(locker) 
     if (_similarity > bestSimilarity)
         bestSimilarity = _similarity;

vs

 if (_similarity > bestSimilarity)
     lock(locker) 
         bestSimilarity = _similarity;

You might want to look at this question/answer. http://stackoverflow.com/questions/394898/double-checked-locking-in-net — John Sobolewski, Oct 20 '11 at 12:05
what are `bestSimilarity` and `_similarity` typed as? this matters, because a read/write is only guaranteed to be atomic for `int` etc (but not for `long`) — Marc Gravell, Oct 20 '11 at 12:26

score 7 · Accepted Answer · answered Oct 20 '11 at 12:00

7

Your first case will work guaranteed. The second case however might break down. You compare, then request a lock while in the meantime another thread already modifies bestSimilarity without you knowing of it making the comparison invalid.

If you want to avoid the lock untill the last minute, you can do a comparison twice. That is, compare, acquire a lock, compare again and only if its still valid, increase the value. Be carefull though with local cache of the value you're comparing against. If you want to do this, you need to have some kind of synchronization there like a MemoryBarrier. This all can get pretty complex so i recommend just lock the whole thing unless you notice performance really is a bottleneck

answered Oct 20 '11 at 12:00

Polity

14,734
2
40
40

2

Double-checked locking works with the current memory model of .NET, but this may change, so better is to use memory barriers here. It's however easy to get them wrong, so the simpler strategy of taking the lock before comparison seems to be more appropriate unless this code presents a performance bottleneck. – Vlad Oct 20 '11 at 12:06
I agree with Vlad. Just use `lock` and only fallback to anything more complicated when you are sure that that `lock` statement is a performance bottleneck. – Steven Oct 20 '11 at 12:24
2

@Vlad actually, a double-check here is *not* necessarily safe, since the OP has not defined what `bestSimilarity` etc **are**. If they are `long` etc then the read/write is not guaranteed to be atomic, so the initial comparison could be comparing a torn value – Marc Gravell Oct 20 '11 at 12:29

score 2 · Answer 2 · answered Oct 20 '11 at 12:00

2

As bestSimilarity is shared, you will need to use the first code segment

answered Oct 20 '11 at 12:00

Ankur

33,367
2
46
72

score 1 · Answer 3 · answered Oct 20 '11 at 12:00

1

The second is not thread safe, another thread could change _similarity after the if test has been performed.

answered Oct 20 '11 at 12:00

vc 74

37,131
7
73
89

score 1 · Answer 4 · answered Oct 20 '11 at 12:03

1

The first solutions is thread safe - the second is not. However you can use double-checked lock to reduce the overhead of acquiring a lock

if (_similarity > bestSimilarity)  
{
    lock(locker) 
    {
         if (_similarity > bestSimilarity)
             bestSimilarity = _similarity;
    }
}

answered Oct 20 '11 at 12:03

Kiril Popov

376
2
9

I will do what you explained. – Nime Cloud Oct 20 '11 at 12:11
2

If the values are `long`, then this is *not* thread-safe, as the reads/writes are not guaranteed to be atomic – Marc Gravell Oct 20 '11 at 12:27
@Kiril: The assumption you make is that there is no other place in the application that writes to either `bestSimilarity` or `_similarity`. If there is (even if it is protected by a `lock`) your assumption fails. – Steven Oct 20 '11 at 12:28

score 1 · Answer 5 · answered Oct 20 '11 at 12:25

You could also do it lock-free:

bool retry;
do
{
    retry = false;
    var copy = Interlocked.CompareExchange(ref bestSimilarity, 0, 0);

    if (_similarity > copy)
    {
        retry = Interlocked.CompareExchange(
              ref bestSimilarity, _similarity, copy) != copy;
    }
} while (retry);

This:

takes a snapshot of bestSimilarity at the start (which I assume is the accumulator)
it then compares our current value (_similarity) to the snapshot (which is stable, as a local)
if it is higher, it swaps in the value but only if the accumulator hasn't changed
if the accumulator has changed, it does the whole thing again

This is fully thread-safe, and lock-free

Where is the correct location to use lock

5 Answers5