On Thu, 25 Jan 2007, Stefan Westerfeld wrote:

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

> |   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.

yes, i meant to have that here, thanks for catching.

> | 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;

oops, right. thanks for catching ;)

> |   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).

the way the code is written above, GCC can auto-vectorize it,
is that true for std::copy as well?

> |         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:

write/read memory barriers are implemented as part of Atomic::uint_get
and Atomic::uint_set. also, volatile variables haven't been updated
here, so there's no need for a barrier at this point.

> |     Atomic::uint_set (&m_wmark, wm);
> |     return orig_length - length;
> |   }

> |   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).

double evaluation isn't a problem here (on variables) and
std::min has other problems (mostly wrg typing).

> | } // Atomic

thanks for the input.

>   Cu... Stefan

---
ciaoTJ
_______________________________________________
beast mailing list
[email protected]
http://mail.gnome.org/mailman/listinfo/beast

Reply via email to