Review of the new ringbuffer

[Stefan Westerfeld <stefan space twc de>]

   Hi!

Here are a few thoughts on Tims new Birnet::Atomic::RingBuffer<T>.

| namespace Atomic {
| 
| template<typename T>
| class RingBuffer {
|   const uint    m_size;

If something has a size, its useful to add an accessor method which
reads the size.

|   T            *m_buffer;

Using vector<T> means doing less things yourself (and less things that
can go wrong).

|   volatile uint m_wmark, m_rmark;

You probably want to add BIRNET_PRIVATE_CLASS_COPY here, as copying (or
assigning a ringbuffer (as you implemented it) will not work.

| public:
|   explicit
|   RingBuffer (uint bsize) :
|     m_size (bsize + 1), m_wmark (0), m_rmark (bsize)
|   {
|     m_buffer = new T[m_size];
|     Atomic::uint_set (&m_wmark, 0);
|     Atomic::uint_set (&m_rmark, 0);
|   }
|   ~RingBuffer()
|   {
|     Atomic::uint_set ((volatile uint*) &m_size, 0);
|     Atomic::uint_set (&m_rmark, 0);
|     Atomic::uint_set (&m_wmark, 0);
|     delete &m_buffer[m_size];

This is wrong (run it in valgrind to see that you are not deleting the
right thing here). The correct code is

  delete[] m_buffer;

|   }
|   uint
|   n_writable()
|   {
|     const uint rm = Atomic::uint_get (&m_rmark);
|     const uint wm = Atomic::uint_get (&m_wmark);
|     uint space = (m_size - 1 + rm - wm) % m_size;
|     return space;
|   }
|   uint
|   write (uint     length,
|          const T *data,
|          bool     partial = true)
|   {
|     const uint orig_length = length;
|     const uint rm = Atomic::uint_get (&m_rmark);
|     uint wm = Atomic::uint_get (&m_wmark);
|     uint space = (m_size - 1 + rm - wm) % m_size;
|     if (!partial && length > space)
|       return 0;
|     while (length)
|       {
|         if (rm <= wm)
|           space = m_size - wm + (rm == 0 ? -1 : 0);
|         else
|           space = rm - wm -1;
|         if (!space)
|           break;
|         space = MIN (space, length);
|         for (int i = 0; i < space; i++)
|           m_buffer[wm + i] = data[i];

Its better to use std::copy, because for some data types (primitive
types), then memmove will be used (template specialization).

|         wm = (wm + space) % m_size;
|         data += space;
|         length -= space;
|       }

Systems with memory barriers need a write memory barrier here. In the
jack driver code this looks like:

    // It is important that the data from the previous writes get committed
    // to memory *before* the index variable is updated. Otherwise, the
    // consumer thread could be reading invalid data, if the index variable
    // got written before the rest of the data (when unordered writes are
    // performed).
    write_memory_barrier();

and:

  void
  write_memory_barrier()
  {
    static volatile int dummy = 0;

    /*
     * writing this dummy integer should ensure that all prior writes
     * are committed to memory
     */
    Atomic::int_set (&dummy, 0x12345678);
  }

I am not sure whether write_memory_barrier is a sufficiently good
implementation on such systems.

|     Atomic::uint_set (&m_wmark, wm);
|     return orig_length - length;
|   }
|   uint
|   n_readable()
|   {
|     const uint wm = Atomic::uint_get (&m_wmark);
|     const uint rm = Atomic::uint_get (&m_rmark);
|     uint space = (m_size + wm - rm) % m_size;
|     return space;
|   }
|   uint
|   read (uint length,
|         T   *data,
|         bool partial = true)
|   {
|     const uint orig_length = length;
|     const uint wm = Atomic::uint_get (&m_wmark);
|     uint rm = Atomic::uint_get (&m_rmark);
|     uint space = (m_size + wm - rm) % m_size;
|     if (!partial && length > space)
|       return 0;
|     while (length)
|       {
|         if (wm < rm)
|           space = m_size - rm;
|         else
|           space = wm - rm;
|         if (!space)
|           break;
|         space = MIN (space, length);
I use std::min where I can, because it doesn't have the typical macro
problems (double args evaluation).

|         for (int i = 0; i < space; i++)
|           data[i] = m_buffer[rm + i];
This should also use std::copy.

|         rm = (rm + space) % m_size;
|         data += space;
|         length -= space;
|       }
|     Atomic::uint_set (&m_rmark, rm);
|     return orig_length - length;
|   }
| };
| 
| } // Atomic

   Cu... Stefan
-- 
Stefan Westerfeld, Hamburg/Germany, http://space.twc.de/~stefan