Most efficent way to use locks for modifying two related fields based on previous values?

Question

I know their are faster\better ways using Interlocked.CompareExchange but I'm just looking for the best way if you are limited to locks.

Joe Albahari says the following:

It’s also possible to safely assign a new ProgressStatus object based on its preceding value (e.g., it’s possible to “increment” the PercentComplete value) — without locking over more than one line of code.

Edit: Updated code based on @drch's answer. Waiting to see if there is a way to do this with a smaller\more efficient lock. This lock has the read, write, and new constructor all in the lock. Was hoping for a smaller\more efficient lock. Thanks.

class ProgressStatus
{
    public readonly int PercentComplete;
    public readonly string StatusMessage;
    public ProgressStatus(int percentComplete, string statusMessage)
    {
        PercentComplete = percentComplete;
        StatusMessage = statusMessage;
    }
}
class Test
{
    readonly object _statusLocker = new object();
    ProgressStatus _status;
    void IncreaseProgress()
    {
        lock (_statusLocker)
          _status = new ProgressStatus(_status.PercentComplete + 10, _status.StatusMessage);
    }
}

IncreaseProgress is only executed by one thread, correct? because if there are multiple threads executing it, you might loose updates, if 2 threads cache the same statusCopy. — aKzenT, Apr 01 '12 at 02:49
@aKzenT No this is assuming many thread calling IncreaseProgress at once. Thanks for pointing out the flaw in the code, this is why I'm looking for the one liner Joe Albahari is talking about. — Aaron Stainback, Apr 01 '12 at 02:51
Have you looked at http://msdn.microsoft.com/en-us/library/dd78zt0c.aspx ? — Ian Mercer, Apr 01 '12 at 03:23
@Hightechrider like I said in the description I'm looking to do this only using a single lock nothing else. Thanks. — Aaron Stainback, Apr 01 '12 at 03:34
@Hightechrider I'm not looking for a solution that is just one line; I'm looking for the absolute most efficient way of using a lock in this situation. I'm assuming this will be a single lock with the code inside the lock being as small and fast as possible. Thanks. — Aaron Stainback, Apr 01 '12 at 05:56

score 1 · Accepted Answer · answered Apr 01 '12 at 04:08

1

In the article you mentioned, that only requiring locking on one line was referencing the advanced technique later in Part 5.

Regarding the code you have above, it is possible to get a race condition between your two locks. Two threads could read the same value, and then write the same value when the desired result would be to have them both increment.

Lock over the entire read and write:

lock (_statusLocker) {
    statusCopy = _status;
    var newStatus = new ProgressStatus(statusCopy.PercentComplete + 10, statusCopy.StatusMessage);
    _status = newStatus;
}

Or simply:

lock (_statusLocker) {
   _status = new ProgressStatus(_status.PercentComplete + 10, _status.StatusMessage);
}

answered Apr 01 '12 at 04:08

drch

3,040
1
18
29

I was under the impression that doing a "new" inside a lock is not that good of a practice. +1 for getting it to one line but I'm still curious if their is a way to do this without the "new" being inside the lock. Thanks. – Aaron Stainback Apr 01 '12 at 04:59
The "new" is not bad-practice. new is increadibly fast. Yesterday I benchmarked that I could do 100m new's per second per CPU core including GC. Don't think about it. – usr Apr 01 '12 at 13:05
1

In any case: correctness over performance. Don't be clever with threading unless you know what you are doing. – usr Apr 01 '12 at 13:06

aKzenT · Answer 2 · 2012-04-01T12:40:09.090

0

IMO, using the code above you can do without any locks, since you are using an immutable class and working only with copies of the data. Reading and writing references are guaranteed to be atomic by the C# spec.

Edit: However thread synchronization is a b***, so I would like to hear some more opinions...

Edit: Also as I pointed out in the comments, there is a flaw in the code if more than one thread calling the method. Updates can get lost in this case.

edited Apr 01 '12 at 12:40

answered Apr 01 '12 at 02:35

aKzenT

7,775
2
36
65

2

You can only avoid locks by using `Interlocked`, and the OP explicitly said that he doesn't want to do that. – CodesInChaos Apr 01 '12 at 18:04
what I wanted to say is that even without using locks, ProgressStatus is always in a good state and there are no smeared values etc. or strange behaviors for someone reading the progress. However as pointed out in the comment their might be lost updates to the progress, but the locks didn't do anything to avoid that. In the original code he could have done without the locks getting the same (wrong) effect than without the locks. – aKzenT Apr 01 '12 at 21:04

Most efficent way to use locks for modifying two related fields based on previous values?

2 Answers2