Is Thread.MemoryBarrier really needed here

[ekalchev]

I was looking at the generated code here https://github.com/DevTeam/Pure.DI/blob/master/readme/accumulators.md

get
    {
      var accumulator44 = new MyAccumulator();
      AbcDependency perBlockAbcDependency4 = new AbcDependency();
      if (_root._singletonXyzDependency41 == null)
      {
        lock (_lock)
        {
          if (_root._singletonXyzDependency41 == null)
          {
            XyzDependency _singletonXyzDependency41Temp;
            _singletonXyzDependency41Temp = new XyzDependency();
            accumulator44.Add(_singletonXyzDependency41Temp);
            Thread.MemoryBarrier();
            _root._singletonXyzDependency41 = _singletonXyzDependency41Temp;
          }
        }
      }

      AbcDependency transientAbcDependency3 = new AbcDependency();
      lock (_lock)
      {
        accumulator44.Add(transientAbcDependency3);
      }
      Service transientService1 = new Service(transientAbcDependency3, _root._singletonXyzDependency41!, perBlockAbcDependency4);
      lock (_lock)
      {
        accumulator44.Add(transientService1);
      }
      return (transientService1, accumulator44);
    }

May be I am missing something, but is that memory barrier really needed here? You have implicit memory barrier at the end of the lock statement.

Also, why there is lock around accumulator44 if it is a local variable?

[NikolayPianikov]

The memory barrier can help in very rare cases. Some processors may not execute instructions in the sequence in which we have defined them. And it is important for us to have the code to create and initialize (in the case of injection via methods, fields or properties) an instance:

XyzDependency _singletonXyzDependency41Temp;
_singletonXyzDependency41Temp = new XyzDependency();
accumulator44.Add(_singletonXyzDependency41Temp);

is always executed before the code when we update the shared state in a class field:

_root._singletonXyzDependency41 = _singletonXyzDependency41Temp;

This is unlikely, but the ECMA specification has a weaker memory model. So it is theoretically possible that some version of .NET on some processor could start behaving this way. Then, some thread could start using the object before it is fully initialized.

Also, why there is lock around accumulator44 if it is a local variable?

accumulator44 can be used when calling Func<> in a thread other than the thread that will create the primary object graph.

[NikolayPianikov]

You can disable thread synchronization. Here is an example.

[ekalchev]

I know the purpose of MemoryBarriers. Usually you use them with lock free programming. I can see you have locks around the shared state. That is why I asked if they are really needed. lock statement generates implicit memory barrier at the end of lock block.

        lock (_lock)
        {
          if (_root._singletonXyzDependency41 == null)
          {
            XyzDependency _singletonXyzDependency41Temp;
            _singletonXyzDependency41Temp = new XyzDependency();
            accumulator44.Add(_singletonXyzDependency41Temp);
            Thread.MemoryBarrier(); // <-- I wonder why you need it here
            _root._singletonXyzDependency41 = _singletonXyzDependency41Temp; // must have some concerns about this line, that can't see why. You can rely on the implicit memory barrier at the end of the lock
          } // <-- Thread.MemoryBarrier(); here by the .net generated assembly code
        }

Here analysis:
accumulator44 is local variable just created in the block scope - it is guaranteed the instance is not yet shared with other threads so no locking is required.

same for perBlockAbcDependency4 - it is local variable so no locking is required.

Inside the lock you create another local variable that doesn't require locks _singletonXyzDependency41Temp

So the only shared state I see is _root._singletonXyzDependency41 and I don't see how the MemoryBarrier do anything useful

You have shared state here _root._singletonXyzDependency41

Thread.MemoryBarrier() // <-- It make sense to add Thread.MemoryBarrier() here, as another thread might already have created the instance
                                       // but you might won't see it in this thread. The behavior won't be errornous and the only drawback of the missing 
                                      // memory barrier is that you might obtain the lock without a need. Once you got the lock you'll see the value
                                     // So this is a trade off for always have some small overhead for the memory barrier, before the first if statement or 
                                     // or bigger overhead but not always (only if another thread happens to assign the value just before the if statement)
if (_root._singletonXyzDependency41 == null)
      {
        lock (_lock)
        {
          if (_root._singletonXyzDependency41 == null)

Here information for the topic https://www.albahari.com/threading/part4.aspx#_Memory_Barriers_and_Volatility

Memory barriers and locking
As we said earlier, Monitor.Enter and Monitor.Exit both generate full fences. So if we ignore a lock’s mutual exclusion guarantee, we could say that this:

lock (someField) { ... }
is equivalent to this:

Thread.MemoryBarrier(); { ... } Thread.MemoryBarrier();

[NikolayPianikov]

lock statement generates implicit memory barrier at the end of lock block.

Yes, you're right `lock' creates barriers. Our barrier is for another. Imagine this scenario:

• Thread 1 performed the first check for null, entered the critical section, checked for null again. Created an instance. Assigned a value to a class field. But has not yet called the Initialize method for this instance.
• Thread 2 performed the first check for null and determined that there is already a value in the class field (this value can get there without waiting for the end of the critical section) and started using it. But it might not have been ready to be used yet.

[NikolayPianikov]

Inside the lock you create another local variable that doesn't require locks

This is indeed the case in your code. But in the general case the state of local variables should be thread-safe as well. For example, if we have delegates that can be called in a thread different from the thread of object composition creation. Imagine that you are not injecting instance creation directly, but injecting a Func<T> factory. In this case, the local accumulator variable can be accessed from any thread that uses the Func<T> factory. The same goes for example for the lazy implementation of IEnumerable<T>: MoveNext() can be called in any thread at the same time. If at this point we try to add values from different threads to the accumulator at the same time, there may be a problem. In this case we should make accumulators thread-safe. But this is not efficient because we can already execute in a critical section and another one is not needed.

[ekalchev]

Are you saying that memory synchronization code is part of a template for code generations and there could be different code generated around that locks, depending what you inject?
And what I see as a code is just happens to be generated for only one use case where the memory barrier is not needed but in different scenario the barrier might be needed?

[NikolayPianikov]

Yes, I mean that there is a general pattern of code generation. But in the example you gave, a memory barrier can also help in rare cases.

Right now the code looks like this:

if (_root._singletonXyzDependency41 == null)
{
  lock (_lock)
  {
    if (_root._singletonXyzDependency41 == null)
    {
      XyzDependency _singletonXyzDependency41Temp;
      _singletonXyzDependency41Temp = new XyzDependency();
      accumulator44.Add(_singletonXyzDependency41Temp);
      Thread.MemoryBarrier();
      _root._singletonXyzDependency41 = _singletonXyzDependency41Temp;
    }
  }
}

If you remove the memory barrier, the code can execute like this:

if (_root._singletonXyzDependency41 == null)
{
  lock (_lock)
  {
    if (_root._singletonXyzDependency41 == null)
    {
      XyzDependency _singletonXyzDependency41Temp;
      _singletonXyzDependency41Temp = new XyzDependency();
      _root._singletonXyzDependency41 = _singletonXyzDependency41Temp;
      accumulator44.Add(_singletonXyzDependency41Temp);
    }
  }
}

If the second process gets the root._singletonXyzDependency41 field (when _root._singletonXyzDependency41 != null) before this instance is added to accumulator44, this process could theoretically not find _singletonXyzDependency41Temp in accumulator44, which is not good. This is unlikely, but it's possible, that's how "Double-checked locking" is used and the first check for null outside of a critical session.

"Double-checked locking" may marginally improve performance when there is a lot of thread competition.