Implementation of a singleton class with atomic members

Question

I'm trying to implement a class to track the performances of a multithreaded code and am running into the following issue.

The tracker should have a single instance, thus the singleton pattern. In order to count object creation and function execution accross all the threads, I thought that using atomic member would be a good idea. But I can't seem to figure out a correct implementation.

This is a minimal code of what I wand to do:

#include <atomic>
#include <iostream>

class Counter
{
public:
    static Counter& instance()
    {
        return instance_;
    };

    void increment()
    {
        counter_++;
    };

private:
    Counter ()
    {
        std::cout << "ctor counter" << std::endl;
    };

    ~Counter ()
    {
        std::cout << "counter_: " << counter_ << std::endl;
        std::cout << "dtor counter" << std::endl;
    }

    static Counter instance_;
    std::atomic<int> counter_{0};
    //int counter_ = 0;
};

Counter Counter::instance_ = Counter();

int main(void)
{
    Counter::instance().increment();
    Counter::instance().increment();
    Counter::instance().increment();

    return 0;
}

If the counter_ variable is an int, it works fine, but would not be thread safe. If it is an atomic<int>, then the compiler tells me this:

   g++ foo.cc
foo.cc:34:38: error: use of deleted function 'Counter::Counter(const Counter&)'
 Counter Counter::instance_ = Counter();
                                      ^
foo.cc:4:7: note: 'Counter::Counter(const Counter&)' is implicitly deleted because the default definition would be ill-formed:
 class Counter
       ^~~~~~~
foo.cc:4:7: error: use of deleted function 'std::atomic<int>::atomic(const std::atomic<int>&)'
In file included from foo.cc:1:0:
/usr/include/c++/7/atomic:668:7: note: declared here
       atomic(const atomic&) = delete;
       ^~~~~~

I'm not sure I quite understand the problem. Any explanation/solution would be much appreciated.

Cheers

Dare I ask why, if this is a singleton, `counter_` it not `static` as well ? (not that I would use a singleton in the first place, but genuinely curious). Further, It would seem to me that whomever is consuming this thing would be just as well (if not better) off to have their own `static std::atomic_int` class member and be done with it. The class `Counter` seem to be trying to solve that already-solved problem. — WhozCraig, Apr 04 '20 at 14:29
@AlexF: The whole point of `std::atomic` is that it *is* thread-safe. It's not UB for multiple threads to be running `counter.fetch_add(1)` in parallel. You don't know which thread is going to see which result, but the act of doing it will establish a modificatino order for that run. — Peter Cordes, Apr 04 '20 at 14:38

Peter Cordes · Accepted Answer · 2020-04-04T15:09:43.683

You could simplify by having std::atomic<int> counter_{0}; simply be a static class member instead of part of each instance. (Since you're ensuring that there's only one instance the class.)

Or if you're only using your "singleton" that way returning a reference to a static object, just make all of its members static so you don't need to even get a pointer to that single instance of it in the first place. Then it can just be a glorified namespace{} with public and private static member functions. I think the only point of a "singleton" is to delay initializing it until after static initialization by using a function-scoped static with a non-constant initializer, but you're not doing that.

The actual problem is in the copy-constructor of your class, which your static initializer uses the way you've written it. It constructs a temporary Counter(); and then copies it to static instance_ variable.

You can compile as C++17 where eliding of that copy is guaranteed (works on Godbolt with g++ -std=gnu++17 with your source untouched), or you can rewrite the initializer

Counter Counter::instance_;    // default construct
Counter Counter::instance_{};  // explicitly default construct

Both of those work with g++ -std=gnu++11

Thank you, that did the trick and I understand better now. The reason why I want to have counter_ in a class is that I want several counters to count different things in various parts of my code, and I'd rather have them all in a single `PerformanceTracker` class. — stanislas.b, Apr 04 '20 at 14:55
@stanislas.b: I meant a `static` class member, just like `static Counter instance_;`. Of course you want to keep it scoped to the class, but it doesn't matter much whether it's beside `instance_` or part of `instance_` — Peter Cordes, Apr 04 '20 at 15:00

Implementation of a singleton class with atomic members

1 Answers1