IndexBuilderService: Almost lock-free iteration

[kiminuo]

Alternative to #11239

Explanation of this PR is here #11239 (comment)

Review is easier with whitespace off.

[kiminuo]

It's generally not OK to put logging commands to critical sections. Especially since the WW logger writes to files (as opposed to buffering log messages to a queue, for example).

So I moved it out of the CS.

[molnard]

Wise consideration!

[kiminuo]

Critical section is now very short and we can then do with the index snapshot whatever we want.

Note that this is the most common case as blocks are produced at the cadence of 1 block ~ 10 minutes. So we have many reads and not many writes.

[molnard]

lock is required here? Copy the reference will point to a snapshot and atomic operation. It should be exactly the same but without waiting.

[molnard]

However, my gut feeling is lock is needed because the order of updating the reference of Index does matter. In that case, you can have a property called IndexSnapshot that is updated before the end of lock sections (add, reorg). And here just use snapshot without the lock.

[kiminuo]

So it's the most correct because that's how synchronization is supposed to be done in .NET. In WW code, there are many instances where we omit synchronization for reads of fields/properties/etc. but it works most of the time¹, so there is this "general consensus" that it's good enough².

Here, I just want get the latest index snapshot we have (not a stale value, which can be returned if there is no lock; also compiler is free to do whatever optimizations it wants based on "no lock -> i can optimize as if single-threaded code").

Then finally, Kristaps shared a chart showing that there are about 50 req/second on the Backend machine but acquiring a lock costs about 20 ns³, so there is imo no reason to go overboard and try to make it faster for the cost of correctness. And if we need more performance, there are other means to reach it (e.g. caching).

Footnotes

1. It depends on a machine architecture as well. Intel gives stronger memory guarratees, ARM chips less. ↩

2. It continues to puzzle me. ↩

3. Really nanoseconds, not millisecond based on https://stackoverflow.com/a/8788511 (old, so maybe it's even better these days). ↩

[molnard]

In WW code, there are many instances where we omit synchronization for reads of fields/properties/etc. but it works most of the time

Reference assignment is guaranteed to be atomic on all .NET platforms.

latest index snapshot we have (not a stale value, which can be returned if there is no lock

I said multiple things but my suggestion was to have a Snapshot and set it INSIDE the lock. I don't know how compiler works but you don't know either. Let me guess: I believe if you have a property and you keep reading it from multiple threads you will get the updated value except when another thread updates it at that very moment. But even in that case, the difference with the lock is that the moment is before or after the lock. If you update at the moment you read you will get a stale value because the writer will wait for the lock to update. So even with a lock, you will have stale values.

But I think here you went too deep because it is not required at all to guarantee 20 ns update will propagate immediately. The next read will read the correct value and that is perfect. It is the same with or without the lock.

[molnard]

I do not want you to change the code. I just want to dismiss this debate permanently, you brought it up too many times and still have not gotten relief from that.

[kiminuo]

Ok, I'll try to explain it. The post is written in a way that I'm in favor of lock (..) {..} and your position is that it's not necessary. It's easier to write the post then.

In WW code, there are many instances where we omit synchronization for reads of fields/properties/etc. but it works most of the time

Reference assignment is guaranteed to be atomic on all .NET platforms.

Yes but you should continue reading https://stackoverflow.com/a/41382380 (talks about Interlocked.Exchange() but lock (..) {} has the same properties) because I'm not talking about atomicity (which I agree with) but I talk about memory visibity (i.e. you get the most recent value of the reference and not some old value; one must be familiar with the concept of memory barriers (see)) & preventing reordering of the instructions.

To understand reading stale values, there is an example here https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile#example and it's explained there

With the volatile modifier added to the declaration of _shouldStop in place, you'll always get the same results (similar to the excerpt shown in the preceding code). However, without that modifier on the _shouldStop member, the behavior is unpredictable. The DoWork method may optimize the member access, resulting in reading stale data. Because of the nature of multi-threaded programming, the number of stale reads is unpredictable. Different runs of the program will produce somewhat different results.

I don't know how compiler works but you don't know either.

You are being presumptious. I have passed a "Compiler internals" course with A. So I guess it's just you.

But I think here you went too deep because it is not required at all to guarantee 20 ns update will propagate immediately. The next read will read the correct value and that is perfect. It is the same with or without the lock.

I'm saying that the cost of using lock is negligible. The way I implemented that - ie - using

lock (IndexLock)
{
    currentIndex = Index;
}

means that you get the latest Index reference always. What you propose (remove lock (...)) means that in the worst case, you can work with some old Index value (set some time in the past), not the latest one.

To illustrate it differently, imagine that a ticket office sells tickets and each ticket has a number. So over the day, they sell tickets with numbers 1, 2, ..., 999, and 1000. With my approach, it's guaranteed that you will get ticket number 1001 when you visit the ticket office and want to buy one. With your approach, you can get 1001 or any older value really (atomicity argument here just says that you can get a ticket number that was actually sold and not gibberish like 9995434534). And you argue that "it works fine in practice".

Real-world example of similar issue (missing volatile read):

• ManyConcurrentAddsTakes_ForceContentionWithToArray fails intermittently on osx-arm64 dotnet/runtime#76501
• https://github.com/dotnet/runtime/pull/78142/files

So even if "mostly it works without locking" is true, I don't get why I should deliberate break my code by removing lock (..) { }.

I just want to dismiss this debate permanently, you brought it up too many times and still have not gotten relief from that.

My goal is to write correct code so that it always works as designed. In practice,

• it costs a few more lines of code (I don't see an issue with that)
• it does not hurt performance (It feels like people are afraid of that for some reason, IDK why.)

whereas the alternative is that one deals with unexplainable issues that no one can easily explain and debug. How is this a win?

I believe if you have a property and you keep reading it from multiple threads you will get the updated value except when another thread updates it at that very moment. But even in that case, the difference with the lock is that the moment is before or after the lock. If you update at the moment you read you will get a stale value because the writer will wait for the lock to update. So even with a lock, you will have stale values.

This part "will get the updated value" is not necessarily true if you read without acquiring a lock. You can get any old value because there is no memory barrier. To say it more clearly, you can actually read a value that was written, eg, 1/5/10/30 minutes ago even though there were writes to that variable in the meantime (that's what stale value typically means in the context of synchronization).

So even with a lock, you will have stale values.

With lock, you get the value that is currently stored in the variable. It's true that you can read and then another thread can immediately change the value. BUT the important part is that you read a well-defined value.

To make more clear, suppose that threads T1 and T2 do this:

• T1 calls MethodA every even second (0, 2, 4, ...)
• T2 calls MethodB every odd second (1, 3, 5, ...)

public class MemoryExample
{
  private int x = 0;
  private readonly object LockObj = new();

  // Only called by Thread 1
  void MethodA()
  {
     lock (LockObj) 
     {
         x++;  
      }
  }

  // Only called by Thread 2
  void MethodB()
  {
      Console.WriteLine(x); // You are not 
  }
}

-> MethodB can very well print 0 all the time. That's allowed behavior for the compiler to do.

[molnard]

So I guess it's just you.

Alright, I will take that back then. 🙏

I read through all that you sent and tried to apply the information to our codebase - with less success. I am bringing up a current example from Wasabi - we had a very similar situation with GetStatus.

#7854

According to what you wrote this code is wrong, because RoundStates reference update is not surrounded by a lock. Is this correct?

[kiminuo]

According to what you wrote this code is wrong, because RoundStates reference update is not surrounded by a lock. Is this correct?

Yes, RoundStates is not correctly synchronized and what I wrote above applies to RoundStates.

[molnard]

Before I reply here, first merge this.

[molnard]

cACK

[molnard]

lock is required here? Copy the reference will point to a snapshot and atomic operation. It should be exactly the same but without waiting.

[molnard]

However, my gut feeling is lock is needed because the order of updating the reference of Index does matter. In that case, you can have a property called IndexSnapshot that is updated before the end of lock sections (add, reorg). And here just use snapshot without the lock.

[molnard]

Wise consideration!

[molnard]

Either in this PR or in the next. Consider using ImmutableDictionary with Key Blockhash and move some functionality here from GetFilterLinesExcluding. I did not think it through deeply but that would be an improvement.

I don't mind if you do it here, I will be able to review.

#11239 (comment)

[kiminuo]

Either in this PR or in the next. Consider using ImmutableDictionary with Key Blockhash and move some functionality here from GetFilterLinesExcluding. I did not think it through deeply but that would be an improvement.

I don't mind if you do it here, I will be able to review.

#11239 (comment)

I would prefer doing that in a next PR. We should do the work based on perf numbers. @kristapsk and @lontivero can get those numbers I believe. I'm not sure if there is an internal tool I can see as well.

[molnard]

We should do the work based on perf numbers.

I guess you will need to run MainNet coordinator on your machine and ask for filters. You can dl the filters from here: https://github.com/molnard/WasabiBackendFilter

[molnard]

I think we should merge this, the discussion around the lock is not related to the functionality. I will try to test this in a few hours.

[molnard]

tACK