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