2
\$\begingroup\$

I need a thread-safe counter that will see frequent increments but will be rarely read. This counter will be used to emit metrics, for example, the hit rate of a ConcurrentDictionary. I could use a simple Interlocked.Increment but it would lead to significant contention when written by multiple threads resulting in performance degradation relative to a ConcurrentDictionary read.

Interlocked.Increment always return the most up-to-date value and I don't have this constraint since the counter will be read every X seconds. The class ThreadInt64PersistentCounter inspired me to write this:

public class ThreadSafeCounter
{
    private readonly ThreadLocal<ThreadLocalCounter> _threadLocalCounter;
    private readonly object _lock;

    private long _deadThreadsCount;

    public ThreadSafeCounter()
    {
        _threadLocalCounter = new ThreadLocal<ThreadLocalCounter>(
            valueFactory: () => new ThreadLocalCounter(this),
            trackAllValues: true);
        _lock = new object();
    }

    public long Value
    {
        get
        {
            lock (_lock)
            {
                return _deadThreadsCount + _threadLocalCounter.Values.Sum(c => c.Count);
            }
        }
    }

    public void Increment() => _threadLocalCounter.Value.Count += 1;
    public void Add(long value) => _threadLocalCounter.Value.Count += value;

    private class ThreadLocalCounter
    {
        private readonly ThreadSafeCounter _parent;

        public ThreadLocalCounter(ThreadSafeCounter parent)
        {
            _parent = parent;
        }
        
        public long Count { get; set; }

        ~ThreadLocalCounter()
        {
            lock (_parent._lock)
            {
                _parent._deadThreadsCount += Count;
                Count = 0;
            }
        }
    }
}

The following benchmark shows good results:

public class IncrementBenchmark
{
    private const int Iterations = 10_000_000;
    
    [Benchmark(OperationsPerInvoke = Iterations)]
    public void WithThreadSafeCounter()
    {
        ThreadSafeCounter counter = new();
        
        Barrier b = new(Environment.ProcessorCount);
        Task.WaitAll(Enumerable.Range(0, b.ParticipantCount).Select(_ =>
            Task.Run(() =>
            {
                b.SignalAndWait();
                for (int i = 0; i < Iterations; i += 1)
                {
                    counter.Increment();
                }
            }))
            .ToArray());
    }

    [Benchmark(OperationsPerInvoke = Iterations)]
    public void WithInterlocked()
    {
        long counter = 0;
        
        Barrier b = new(Environment.ProcessorCount);
        Task.WaitAll(Enumerable.Range(0, b.ParticipantCount).Select(_ =>
            Task.Run(() =>
            {
                b.SignalAndWait();
                for (int i = 0; i < Iterations; i += 1)
                {
                    Interlocked.Increment(ref counter);
                }
            }))
            .ToArray());
    }
}
Method Mean Error StdDev
WithThreadSafeCounter 13.63 ns 0.271 ns 0.654 ns
WithInterlocked 159.31 ns 0.233 ns 0.206 ns

Though I'm not exactly sure whether it's completely correct. Does the lock guarantee that the ThreadSafeCounter._deadThreadsCount and ThreadLocalCounter.Count will be visible at the same time? I'm not sure if ThreadLocal will ever remove its reference on the ThreadLocalCounter of a dead thread, so will the finalizer of ThreadLocalCounter ever be called?

What do you think about this solution and do you know any other alternative?

\$\endgroup\$
2
  • \$\begingroup\$ the counter will be read every X seconds << why don't you emit events with the current value and do not allow direct read? \$\endgroup\$ Commented Sep 14, 2023 at 9:13
  • \$\begingroup\$ That's definitely a possibility though I'm not sure it would simplify the problem. \$\endgroup\$
    – Greg
    Commented Sep 14, 2023 at 13:35

1 Answer 1

2
\$\begingroup\$

Interlocked.Increment is an atomic operation, and as such should not cause (locking) contention. In other words; there are no mutexes or locks involved. Edit: As pointer out in the comment, memory contention still may occur.

Atomic operations, at least on x86 and x86_64 platforms, are hardware based.

See the docs.

Interesting read: here.

ThreadLocal storage doesn't neet atomicity, and also does not need any memory reordering or memory barriers, as the variable is specific to each thread. These benchmarks can't be compared on atomicity vs locks alone. Your ThreadSafeCounter allows for memory reordering to occur, which might yield performance increases from a cpu optimization view (but only for this specific benchmark).

This code also benches by Environment.ProcessorCount amount of tasks without checking how large the Task threadpool is. A Task.Run does not necessarily equal a single / one thread, it's just an async operation that is queued to be picked up by the threadpool.

The ThreadSafeCounter does indeed run much faster, and yield the same (correct) result. Why that is would require a more in-depth analysis.

\$\endgroup\$
2
  • \$\begingroup\$ "Interlocked.Increment should not cause contention." Even if it doesn't cause lock contention, it shoud cause memory contention at the hardware level. "A Task.Run does not necessarily equal a single / one thread". This is true. To cope with that I've used a Barrier so I get the guarantee that Environment.ProcessorCount number of threads are all waiting on the Barrier before starting the increments. "Your second case". I'm a bit confused about what you refer as first/second case. \$\endgroup\$
    – Greg
    Commented Sep 20, 2023 at 21:12
  • \$\begingroup\$ Thanks for the feedback @Greg, I think you're right, I edited my answer to incorporate new info and clarify. \$\endgroup\$
    – Raf
    Commented Sep 21, 2023 at 7:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.