3
\$\begingroup\$

This question is a follow up in regards to the comments suggested in a previous post regarding a Ringbuffer implementation in C++ for audio processing.

Ring buffer for audio processing

template <typename T>
class Array {
public:
    Array();

    Array(std::size_t const& cap);

    Array(Array<T> const& other);

    Array<T>& operator=(Array<T> other);

    virtual ~Array();

    void swap(Array<T>& first, Array<T>& second);

    std::size_t const& Capacity()const;

    void Resize(std::size_t cap);

    T& operator[](std::size_t const& index);

    T const& operator[](std::size_t const& index)const;

protected:
    std::size_t capacity;
    T* data;
};

template<typename T>
Array<T>::Array():capacity(0),data(nullptr){}

template<typename T>
Array<T>::Array(std::size_t const& cap):capacity(cap),data(capacity ? new T[capacity]() : 0){}

template<typename T>
Array<T>::Array(Array<T> const &other):capacity(other.capacity),data(capacity ? new T[capacity]() : 0){
    std::copy(other.data, other.data + capacity, data);
}

template<typename T>
Array<T>& Array<T>::operator=(Array<T> other){//implicit copy
    if (this != &other) {
        swap(*this, other);//and swap 
    }
    return *this;
}

template<typename T>
Array<T>::~Array(){
    delete [] data;
    capacity=0;
}

template<typename T>
void Array<T>::swap(Array<T>& first, Array<T>& second){
    std::swap(first.capacity, second.capacity);
    std::swap(first.data, second.data);
}

template<typename T>
std::size_t const& Array<T>::Capacity()const{
    return capacity;
}
template<typename T>
void Array<T>::Resize(std::size_t cap){
    Array<T> temp(cap);
    swap(*this, temp);
}

template<typename T>
T& Array<T>::operator[](std::size_t const& index){
    return this->data[index];
}

template<typename T>
T const& Array<T>::operator[](std::size_t const& index)const{
    return this->data[index];
}

Split Code into two sections for easy reading.

#define MAKE_POW_2(number)( pow(2, ceil(log(number)/log(2))))//round up to the nearest power of two

template<typename T>
class Ringbuffer{
public:
    Ringbuffer();

    Ringbuffer(std::size_t cap);

    ~Ringbuffer();

    Ringbuffer(Ringbuffer<T> const& other);

    Ringbuffer<T>& operator=(Ringbuffer<T> other);

    bool Read(T& location);

    bool Write(T const& value);

    inline bool IsFull()const;

    inline bool IsEmpty()const;

    void swap(Ringbuffer<T>& first, Ringbuffer<T>& second);

    std::size_t const& Count()const;

    std::size_t const& Capacity()const;

    void Resize(size_t cap);

    void Initialize();
protected:
    inline void next_index(size_t& current)const;//increment index variable with wraparound

    Array<T> buffer;
    std::size_t read;
    std::size_t write;
    std::size_t count;
    std::size_t mask;
};

template<typename T>
Ringbuffer<T>::Ringbuffer():buffer(),read(0),write(0),count(0),mask(0){}

template<typename T>
Ringbuffer<T>::Ringbuffer(std::size_t cap):buffer(MAKE_POW_2(cap)),read(0),write(0),count(0){
    mask = buffer.Capacity()-1;
}

template<typename T>
Ringbuffer<T>::~Ringbuffer(){
    read=0;
    write=0;
    count=0;
    mask=0;
}

template<typename T>
Ringbuffer<T>::Ringbuffer(Ringbuffer<T> const& other):buffer(other.buffer),read(other.read),write(other.write),count(other.count){}

template<typename T>
Ringbuffer<T>& Ringbuffer<T>::operator=(Ringbuffer<T> other){
    if (this != &other) {
        swap(*this,other);
    }
    return *this;
}

template<typename T>
bool Ringbuffer<T>::Read(T& location){
    if (!IsEmpty()) {
        location = buffer[read];
        next_index(read);
        --count;
        return true;
    }
    return false;
}

template<typename T>
bool Ringbuffer<T>::Write(T const& value){
    if (!IsFull()) {
        buffer[write]=value;
        next_index(write);
        ++count;
        return true;
    }
    return false;
}

template<typename T>
inline bool Ringbuffer<T>::IsFull()const{
    return (count == buffer.Capacity());
}

template<typename T>
inline bool Ringbuffer<T>::IsEmpty()const{
    return (count==0);
}

template<typename T>
void Ringbuffer<T>::swap(Ringbuffer<T>& first, Ringbuffer<T>& second){
    std::swap(first.buffer,second.buffer);
    std::swap(first.read,second.read);
    std::swap(first.write,second.write);
    std::swap(first.mask, second.mask);
}

template<typename T>
std::size_t const& Ringbuffer<T>::Count()const{
    return count;//number of elements currently being stored
}

template<typename T>
std::size_t const& Ringbuffer<T>::Capacity()const{
    return buffer.Capacity();
}

template<typename T>
void Ringbuffer<T>::Resize(size_t cap){
    buffer.Resize();
    Initialize();
}

template<typename T>
void Ringbuffer<T>::Initialize(){
    read=0;
    write=0;
    count=0;
    mask=0;
    for (int i=0; i<buffer.Capacity(); ++i) {
        buffer[i] = T();
    }
}

template<typename T>
inline void Ringbuffer<T>::next_index(size_t& current)const{
    current = (current+1) & mask;
    //bitwise modulo increment (only works on pow 2 size arrays which is why we force pow 2 size in constructor)
}

The new version of the code assumes a single threaded environment and has been written as a template to improve versatility.

The Flush function has been re-named Initialize in order to make its purpose clearer.

The Full and Empty functions have been re-named as IsFull and IsEmpty in order to make their purposes clearer.

As always I am looking to resolve any possible issues in the code.

\$\endgroup\$
  • \$\begingroup\$ Turn that MAKE_POW_2 macro into a private class method. Remember that macros don't respect scope, so you are polluting the global namespace with it when it could be a properly class-scoped function. \$\endgroup\$ – glampert Jun 10 '15 at 17:46
7
\$\begingroup\$

Warning There is already a std::array

You may want to be careful because there is already a standard array. You don't want want your user to be confused. I don't think it will be a problem but you should keep it in mind.

Don't re-write container classes.

Your array class is basically a simplification of std::vector. BUT it does not have all the features that make vector efficient. Why not use the standard container? If you don't want to expose the whole vector interface just wrap it and expose the methods you actually want.

Your style is a bit cramped

template<typename T>
Array<T>::Array():capacity(0),data(nullptr){}

Sure that works. But a bit of white space is not going to kill you and it will make it easier for humans to read. At least put a space after the comma and colon.

Personally I would use up a couple of lines:

template<typename T>
Array<T>::Array()
    : capacity(0)
    , data(nullptr)
{}

Prefer nullptr over zero

template<typename T>
Array<T>::Array(std::size_t const& cap)
    : capacity(cap)
    , data(capacity ? new T[capacity]() : 0)
{}                                       ^^^^^^   use nullptr here

The use of nullptr will convey your intent better.

Expensive Copy

template<typename T>
Array<T>::Array(Array<T> const &other):capacity(other.capacity),data(capacity ? new T[capacity]() : 0){
    std::copy(other.data, other.data + capacity, data);
}

The problem here is that if the construction and copy of T are both expensive then you are paying the cost twice. There is no simple solution as a change to your class will have a ripple affect.

 new T[capacity];   // Create `capacity` objects and calls the constructor.
                    // for T on each object.

 // Now you loop over the data and 
 // and call the assignment operator T on each member.
 std::copy(other.data, other.data + capacity, data);

With a bit of work you can separate the allocation of memory from the construction. Thus only call the copy constructor on each member (thus reducing the overall load). But this takes a bit more work than I want to do just now.

Alternatively you can std::vector<T> and this problem goes away.

Copy and Swap

No need to test for self assignent when using the copy and swap idium.

template<typename T>
Array<T>& Array<T>::operator=(Array<T> other){//implicit copy
    if (this != &other) {
        swap(*this, other);//and swap 
    }
    return *this;
}

Notice that other is passed by value. Thus it is its own value. There is no way that other will ever be this (even if self assignment happened) because we have made a copy of the original object.

template<typename T>
Array<T>& Array<T>::operator=(Array<T> other)
{
    swap(*this, other);
    return *this;
}

Swap usually takes one parameter

You have conflated the swap function with the swap method.

template<typename T>
void Array<T>::swap(Array<T>& first, Array<T>& second){
    std::swap(first.capacity, second.capacity);
    std::swap(first.data, second.data);
}

Normally you see swap defined like this.

class Array
{
    public:
        void swap(Array& other) noexcept   // Important to use noexcept
        {
             // The one place were using is good.
             // Get used to doing it this way.
             // Even when not strictly required. Because if you change the
             // types of your members then this will continue to work.
             //
             // By having using here.
             // The compiler will look up a specific swap by using
             // argument dependent look-up to find the correct swap in the
             // correct namespace. If one does not exist then it can use
             // the generic one defined in the standard namespace.
             using std::swap;
             swap(capacity,   other.capacity);
             swap(data,       other.data);
        }
}
void swap(Array& lhs, Array& rhs) {
    lhs.swap(rhs);
}

Notice you should also mark your swap operations noexcept.

Don't do busy work in the destructor

template<typename T>
Array<T>::~Array(){
    delete [] data;
    capacity=0;           //  This line is not needed.
                          //  After this methdo finishes the object no longer exists.
                          //  So there is no concept of a variable called capacity.
}

Don't use this

return this->data[index];

This is a sign you have done a bad job naming your variables and you need to explicitly differentiate members from local scope variables. This is not the case so simply return what you mean.

return data[index]

Note:

Most of what I said above also applies RingBuffer. I will not repeat myself. So this next section are only the other parts.

No need to use macros

#define MAKE_POW_2(number)( pow(2, ceil(log(number)/log(2))))

You can get the same expressive power with functions. Also functions are part of the language so are more strictly checked for correctness.

template<typename T>
inline auto makePow2(T number)
{
    return pow(2, ceil(log(number)/log(2))));
}

Dont create destructors that are not needed:

template<typename T>
Ringbuffer<T>::~Ringbuffer(){
    read=0;
    write=0;
    count=0;
    mask=0;
}

Ring Buffer has no resource management. So remove its destructor. It does nothing useful anyway.

Don't use inline were not needed

inline bool IsFull()const;

inline bool IsEmpty()const;

Does not help here just muddies up what I need to read to see the interface. If the compiler needs inline it will let you know by complaining.

Don't create public functions that have no use.

inline bool IsFull()const;
inline bool IsEmpty()const;

Is it useful for the application to know this? It is already checked by the Read/Write methods. I don't actually know this is a question you need to ask. Can the application do anything useful with this information.

If not then leave them out of the public interface. It is a lot easier to add than remove functions from a public interface once it has been used.

This is one you definately should not be in the public interface.

void Initialize();

The constructor does the initialization. It may defer to an initialize method but that should not be publicly available. But maybe you mean reset?

Don't re-do work

void Ringbuffer<T>::Initialize(){
    ...
    for (int i=0; i<buffer.Capacity(); ++i) {
        buffer[i] = T();
    }

The buffer is an Array. The array already initialized all those members. You don't need to re-initialize them

\$\endgroup\$
  • \$\begingroup\$ the 'Initialize' should most likely be renamed as 'reset' and the 'IsEmpty' and 'IsFull' do play a role outside of the class which is why I have left them public. The copy things I will take a look at later when I have more time. As far as re-writing the array class... To my understanding the 'std::array' is a fixed length array via template parameters which was not desirable in my situation and the 'std::vector' is far to slow upon insertion for my purposes. I have profiled a version using that style of implementation and it weighed down the overall execution time of my program. \$\endgroup\$ – Alex Zywicki Jun 10 '15 at 21:03
  • \$\begingroup\$ 'std::vector' is far to slow upon insertion for my purposes. That I do not believe. Even in the slightest. Your implementation is not better than std::vector. This means your test are not measuring what you think. Using std::vector<> has a speed equivalent of using raw array and if used properly is faster than raw arrays because it does not needlessly create objects its does not use. \$\endgroup\$ – Martin York Jun 10 '15 at 22:18
  • \$\begingroup\$ Maybe we should review your profile tests. I am sure we can find a problem with the tests. \$\endgroup\$ – Martin York Jun 10 '15 at 22:58
  • \$\begingroup\$ I would love to, but unfortunately the results and the code got lost when my ssd crashed a while back. Convenient right? \$\endgroup\$ – Alex Zywicki Jun 10 '15 at 23:43
  • \$\begingroup\$ Then change your Array into a wrapper around std::vector and watch your speed of inserts increase by factor of ten. Seriously std::vector<> is faster than your code for all but the most basic types (like int). In which case std::vector<> will equal the speed of your array. \$\endgroup\$ – Martin York Jun 11 '15 at 0:28
1
\$\begingroup\$

In addition to @Loki Astari's excellent analysis, I would add the following:

Naming

Your member variable names could be better. Generic names like buffer aren't real useful. What is it a buffer of? In this case, audio samples, so call it audioSamples.

What do the member variables read and write represent? They look like offsets into the buffer of audio samples where the next sample will be read from or written to. So maybe call them something like nextReadIndex and nextWriteIndex, or something like that. As they stand, they seem like they could be booleans describing which operation is happening, or something like that.

Cleverness

The mask variable appears to just be used for a clever hack to get around using the modulus operator. That's fine if you know what it is, but many people won't understand it and it will seem like magic to someone maintaining this code in the future. The compiler should be able to optimize a % into an & if appropriate, and if you use the % operator you get more readable code and can eliminate an instance variable.

Efficiency

Is a user of this class really going to Read() or Write() a single sample at a time? That's probably not as efficient as it could be (though maybe your specific use case warrants it?). You might want to have a method to read/write a series of samples in bulk.

Also, instead of a Read() method, would an iterator make more sense? This is C++ after all! Though, maybe that wouldn't work in a ring buffer, as the concept of begin() and end() are fluid. Just a thought. Scratch that - an iterator's a terrible idea. There are rules about being able to do math on them, etc. that just don't work here.

Language Stuff

What is the point of the default constructor? A user who attempts to use it will end up with a buffer they cannot write to or read from, as its size is 0. To actually use it, they'll have to immediately call Resize() which will reinitialize all of the member variables and set the size of the buffer. Just require them to use one of the other constructors by making the default constructor private.

\$\endgroup\$
  • \$\begingroup\$ The single sample read and write are important because i need the ability to load, process and store one sample of audio data at a time. I was considering adding a bulk read and write in the future. The bitwise increment has shown performance increases in my older versions of the code, I don't have hard profiling numbers but rather in the overall amount of data I have been able to process when using that implementation \$\endgroup\$ – Alex Zywicki Jun 11 '15 at 13:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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