1

Does this code safely implement the double-check idiom using C++11 atomic? I saw in "The C++ Programing Language 4th ed." an example that uses atomic<bool> and I did my best to keep things equivalent but I'm not confident. Also, can this be improved?

I'd like to avoid call_once because of the storage overhead of once_flag.

This "LazyArray" was written as a memory-reduction effort to replace arrays with minimum changes to client code (only a small subset of the elements are expected to be accessed). Access to the array from multiple threads is a fact of life and locking broadly would be problematic due to performance.

/**
 * Lazy creation of array elements.
 * They will only be created when they are referenced.
 *
 * This "array" does not support iteration because it can have holes
 *
 * Array bounds isn't checked to keep it fast.
 * Destruction of the array destroys all the T objects (via the unique_ptr d'tor)
 */

template<class T, size_t size>
class LazyArray
{
    typedef LazyArray<T, size> mytype;
public:
    // copying is not allowed (unlike regular arrays)
    LazyArray(const LazyArray&) = delete;
    LazyArray& operator=(const LazyArray&) = delete;

    LazyArray(){}

    T& operator[](size_t i)
    {
        return at(i);
    }

    const T& operator[](size_t i) const
    {
        return const_cast<mytype *>(this)->at(i);
    }
private:
    using guard = std::lock_guard<std::mutex>;

    // get T object at index i by reference
    T& at(size_t i) // only non-const variant is implemented, const version will use const_cast
    {
        auto &p = m_array[i];
        std::atomic<T*> ap(p.get());

        if(!ap) // object not created yet
        {
            guard g(mtx);
            ap = p.get();
            if(!ap)
                p.reset(new T);
        }
        return *p;
    }

    std::unique_ptr<T> m_array[size];
    std::mutex mtx;
};
CplusPuzzle
  • 298
  • 2
  • 9
  • 1
    Keep in mind this only makes *any* sense at all when `sizeof(T)` is much greater than `sizeof(unique_ptr)` (typically the size of a pointer, e.g. 8 bytes). You already have an array of `size` unique_ptr objects using up space. Introducing another level of indirection is also not good for performance, and could lead to much worse locality if you ever iterate over sequential elements of the array. – Peter Cordes Mar 16 '20 at 10:22
  • Ideally you'd *allocate* contiguous memory for an array of `T` directly, but not touch it so the OS's lazy allocation mechanism could do its magic for you (leaving fresh virtual memory pages from the OS lazily zeroes / COW mapped to the OS's zero page) until the first read or write. But then you'd need to know when to use placement-new to construct a new element via separate bookkeeping or a sentinel field in T. i.e. know some part of T that must not be `0` in an already-constructed object. – Peter Cordes Mar 16 '20 at 10:24
  • You should implement your non-const function using the const one and a `const_cast`, not the other way around. It's fine for a non-const function to cast away a const. The issue in your current code is undefined behavior due to member modification via the `const_cast` in your const function. – AVH Mar 16 '20 at 10:24
  • @Darhuuk, maybe the const variant should be removed in this case because if the LazyArray is const, it means that each `unique_ptr` is const as well and can't be safely reset right? There is no const calling the non-const variant, both call `at()` – CplusPuzzle Mar 16 '20 at 10:41
  • 1
    @CplusPuzzle My point is that your const function modifies members of the class (in this case the mutex, in the non-const function `at`). Normally that wouldn't compile. Because of the `const_cast` it does, but now it's undefined behavior. – AVH Mar 16 '20 at 10:47

1 Answers1

1

No, in theory.

The check:

        std::atomic<T*> ap(p.get());
        if(!ap) // object not created yet

Should be acquire corresponding to this release:

                p.reset(new T);

In fact it is not.

How it may fail:

  1. Some parts of object construction new T may be visible to the other thread after it sees p assigned with new value
  2. The assignment p.reset may be torn since it is non-atomic

Possibly, in some platforms.

  1. This happens either if stores can be reordered after other stores (does not happen on x86) or if compiler decides to swap stores these stores itself (not likely)
  2. Pointer-sized variable writes are not torn

Use std::atomic to fix both issues:

   std::atomic<T*> m_array[size];

Sure you have to delete T* manually. I'd suggest creating atomic_unique_ptr based on std::atomic<T*> and using it there

Alex Guteniev
  • 12,039
  • 2
  • 34
  • 79
  • Thanks for this @Alex. Isn't it guaranteed that `p` gets the address of the new `T` only after the `T` object is fully constructed? – CplusPuzzle Feb 27 '21 at 05:45
  • @CplusPuzzle, no. Stores can be ordered after other stores. On platforms with weaker ordering, like ARM, it may happen due to the hardware behavior. See the table here: https://en.wikipedia.org/wiki/Memory_ordering#Runtime_memory_ordering It may hypothetically happen due to compiler optimization, though not very likely, see "Optimization under as-if" https://en.wikipedia.org/wiki/Memory_ordering#Optimization_under_as-if. – Alex Guteniev Feb 27 '21 at 08:25
  • OK, thanks @Alex. Is it correct to say that it's guaranteed that only one object will be created per array index and that invalid memory can't be accessed in case the array elements are plain data? – CplusPuzzle Feb 28 '21 at 06:36
  • Only one object - yes. Invalid memory can't be accessed - no, it can due to above mentioned reordering. Plain data reduces the practical chance for this to happen, but does not eliminate theoretical possibility. For example, debug `new` may initialize to some pattern, and this initialization may move past assignment of `p`. – Alex Guteniev Feb 28 '21 at 09:16