C# - Locking getter and setter of counter if multiple threads increment counter

Question

Multiple threads are incrementing the two counters in below code but only one thread will get the value of counters. Now how to safely apply the lock on the counters while reading the counters value.

Is Interlocking needed in increment methods? is it good for performance? locking in getStats would be sufficient to get the counters? Also while i am getting the counters can any other threads increment the counter by calling the increment method? if yes how to mitigate that?

public sealed class StatisticsCounter
    {
        private static StatisticsCounter instance = null;
        private static readonly object Instancelock = new object();
        private volatile int Counter1 = 0;
        private volatile int Counter2 = 0;

        private StatisticsCounter()
        {
        }

        public static StatisticsCounter GetInstance
        {
            get
            {
                if (instance != null)
                {
                    lock (Instancelock)
                    {
                        if (instance == null)
                        {
                            instance = new StatisticsCounter();
                        }
                    }
                }
                return instance;

            }
        }

        public void IncrementCounter1()
        {   
            //is interlocking required? or can we do += 1. 
            //performance impact of interlocked
            Interlocked.Increment(this.Counter1)
        }
        
        public void IncrementCounter2()
        {
            Interlocked.Increment(this.Counter2)
        }

  

        public string GetStats()
        {
            string stats = null;
            //lock here will suffice?
            lock (Instancelock)
            {
                stats = string.Format("Counter1 : {0} , Counter2 : {2}", Counter1, Counter2);
                //reset
                reset();
                return stats;
            }
        }

        private void reset()
        {
            Counter1 = 0;
            Counter2 = 0;
        }
    }

Answer 1

In GetStats, the lock does not really do anything currently. But "thread safety" depend on what your requirements are.

A lock would be required if you need all the returned stats strings to equal the number of calls to the increment methods. In the current version a increment call may occur after the variables have been read, but before they have been reset. Using a lock is arguably also safer since they are just easier to understand than lock free code. If you use a lock you need to lock the same object in both the increment methods and the GetStats method, and you can remove the interlocked and volatile code, since they would not be needed if you only access the variables inside locks.

As a rule of thumb, taking a uncontested lock is fairly fast. Both your GetStats and increment methods are very short, so assuming your worker threads does other things than just incrementing the counters, I would expect the performance overhead to be fairly small. The general recommendation is to measure first, and only optimize if the performance is insufficient.

But even if the individual accesses to the count-variables are thread safe, that does not mean they will run in any particular ordering. Other synchronization might be required to ensure the calls are done in any specific order.

Also, as mentioned in the comments, just use Lazy<T> to instanciate your singleton:

private static readonly Lazy<StatisticsCounter> lazy = new (() => new StatisticsCounter());
public static StatisticsCounter GetInstance() => lazy.Value; 

Assuming it actually has to be a singleton. In most cases it is better avoid global variables and just inject dependencies as constructor parameters.