1

Please forgive me if the question is asked before, as I cannot find a good keyword to search for it. So if you have any recommendation for the keyword for the problem below, please let me know.

So the question is: suppose I have a class

class Test
{
   public int testProp;
}

When I do something like

var testInstance = new Test();
testInstance.testProp += 10;

will the address of the testProp still be the same as it is before the + operation? When I tried with the Visual Studio disassembly window, I see that it seems to copy the new value to the same address (Please correct me here as I'm in no where familiar with Assembly lang).

I'm asking because in case of multiple thread access the property and increase its value concurrently

var list = new List<Task>();
for (var z = 0; z < 2000; z++)
{
   list.Add(Task.Run(() =>
      {
         testInstance.testProp+=10;
      }));
}

sometimes it would give me 20000, but sometimes only 19990 or less, so I'm quite confused if the address of testProp is changed, and if not how is the value of the address is read that makes the sum less than 20000 in this case?

Dan Le
  • 308
  • 2
  • 10
  • 1
    The address of the variable isn't changed. The value that is stored at that address changes. – Ken White Jun 23 '20 at 18:28
  • 1
    Well testProp is a `field` not a property, big difference. It's a big sin to make a `field` public actually. The easiest way to solve your problem is to change `public int testProp` to `public volatile int testProp`. – Legacy Code Jun 23 '20 at 18:54
  • @Legacy Code yes I know, that's just some quick sample code that I tried out. – Dan Le Jun 23 '20 at 19:00
  • @LegacyCode just to be explicit in all the places where you've claimed this: no, `volatile` does not fix this – Marc Gravell Jun 23 '20 at 19:26
  • @MarcGravell I can't obviously edit my comment to strike it through... – Legacy Code Jun 23 '20 at 19:41

3 Answers3

3

The memory address doesn't change. However, the += operator is not atomic. This means, it will not read the value, increment it and replace the original value in a single operation. Imagine the sequence of incrementing as this:

READ value
INC value
WRITE value

With multiple threads, these can be mixed up:

THREAD A READ value
THREAD B READ value  # same value read
THREAD B INC value
THREAD B WRITE value
THREAD A INC value
THREAD A WRITE value   # does not take into account value written by B

So, when you think it should have been incremented twice, in fact it was only incremented once. As all the work done in Thread B is overwritten by thread A.

Garr Godfrey
  • 8,257
  • 2
  • 25
  • 23
2

The sync between multiple threads has nothing to do with which memory address it writes to. To perform multi threading safe atomic operations use the Interlocked class, i.e. Interlocked.Increment.

Krumelur
  • 31,081
  • 7
  • 77
  • 119
1

The += operator, when used with an int prop or field, is doing multiple operations:

  • get
  • add
  • set

This means that it is not atomic, and race conditions can cause lost changes. If you want an atomic increment, you can use:

Interlocked.Add(ref testInstance.testProp, 10);

This has more overhead, but it will give you the correct result. If you don't have direct access to the field, you would have to synchronize, probably via lock.

Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • Thanks for the great answer. I did a search on atomic operation and have learnt a lot. – Dan Le Jun 23 '20 at 18:42
  • Or use volatile :) https://learn.microsoft.com/en-za/dotnet/csharp/language-reference/keywords/volatile – Legacy Code Jun 23 '20 at 18:45
  • 2
    @LegacyCode using `volatile` does **not** make things atomic - you can get the exact same problems; `volatile` (as a side-effect of the thing it *actually* does) simply prevents a scenario where the field is repeatedly read from a register rather than the actual field; you can still get lost updates when using `volatile` – Marc Gravell Jun 23 '20 at 18:58
  • Yes but it solves his small loop problem there. But you are right the grand solution would be using a lock + volatile. – Legacy Code Jun 23 '20 at 19:02
  • 1
    @LegacyCode no, there is no requirement to use lock + volatile; a lock is explicitly a memory barrier (full fence), which is *more* than what volatile achieves; if you are using lock + volatile, then you probably don't understand volatile (to be fair, most people don't; I would struggle to articulate it clearly, other than the useful side effect that everyone thinks of). And no, it doesn't solve the loop problem either - as the false values *aren't necessarily coming from registers* (in fact, I'd wager that they aren't) - this looks like a simple race condition, nothing to do with `volatile` – Marc Gravell Jun 23 '20 at 19:07
  • Well its 2 or multiple concurring threads getting the same value from the same field, incrementing it and writing it back. The lock does not garanty that I get the actual value of the int right? I could be unlucky and I get the cached value. – Legacy Code Jun 23 '20 at 19:22
  • @LegacyCode yes, absolutely it is; which doesn't change anything I said; that doesn't mean it is related to volatility; you could get this exact effect on an in-order CPU (volatility is all about CPU instruction reordering) – Marc Gravell Jun 23 '20 at 19:24
  • @Marc Gravell I agree with using just only lock. Actually I found this question also from SO that has the same problem and also good answers to each approach (volatile, lock and Interlocked) https://stackoverflow.com/questions/154551/volatile-vs-interlocked-vs-lock. And volatile only guarantees that the value is not cached, not preventing the write operation to override each other. – Dan Le Jun 23 '20 at 19:26
  • @DanLe that's a great link, thanks - for LegacyCode's benefit: please read the first few lines of the accepted answer, where "volatile" is listed as "Worst (won't actually work)" – Marc Gravell Jun 23 '20 at 19:27
  • @MarcGravell Sorry... I have to always remember not to press enter when typing in the comments... It always kills me everytime – Legacy Code Jun 23 '20 at 19:28
  • @MarcGravell `It doesn't stop them at all from interleaving their reads and write operations which is the problem you are trying to avoid.` Ok :) I see... It does not really solve his problem. – Legacy Code Jun 23 '20 at 19:30
  • 1
    @DanLe the fun thing is: the whole "the value is not cached" thing is actually the side-effect of volatile, not the intent; the *intent* is all about CPU instruction re-ordering :) – Marc Gravell Jun 23 '20 at 19:30
  • 1
    Good to know... I always though the not cached part is the reason for volatile in the first place. – Legacy Code Jun 23 '20 at 19:34
  • @MarcGravell yes, I'm actually "half" quoting from the other answer just to see that making the value not cached does not mean the write operation is guaranteed to not overriding each other. Thanks again for your great answer :) – Dan Le Jun 23 '20 at 19:36