Re: Lockfree ringbuffer review

[Tim Janik <timj imendio com>]

On Tue, 30 May 2006, Stefan Westerfeld wrote:

>   Hi!

well, for the most part, i can't make too much sense of the code,
because i don't know how it's expected to be used which is also not
documented anywhere.
a blurb about what thread model and communication patterns you're
using seems to be neccessary, and it probably should go into the
code directly.
also, when doing that, you should add comments next to functions
explaining which thread/entity/etc. is supposed to call it.

so, i'm going to make mostly stylistic comments here. in particular
i can't say anything about the atomic ops. i just noted that you're
only using int_set()/int_get() instead of the usual int_get()/int_cas()
(note that int_set() is just provided for initialization purposes).
and that doesn't make too much sense to me, at least not without the
cmmunication model blurb i sked for above.

> template<class T>
> class FrameRingBuffer {
>  //BIRNET_PRIVATE_COPY (FrameRingBuffer);
> private:
>  vector<T> buffer;
>  typedef typename vector<T>::iterator BufferIterator;
>  int read_frame_pos;
>  int write_frame_pos;

these pointers should be volatile if you mean to access them atomically.

>  guint elements_per_frame;

members should be prefixed with "m_".


> public:
>  FrameRingBuffer (guint n_frames = 0,
> 		   guint elements_per_frame = 1)
>  {
>    resize (n_frames, elements_per_frame);

to avoid shadowing of members via local variables or function arguments
like here.

>  }
>  /**
>   * checks available read space in the ringbuffer
>   *
>   * @returns the number of frames that are available for reading

i think read_space() is a very irritating funciton name for this,
just like write_space() below. the "verb object" form indicates that
this function *does* something, i.e. read/write.
simply renaming to get_read_space() would fix that. but then, "space"
si allmost a dummy word in that it could mean nearly anything (e.g.
bytes, bits, pages, whatever). afaiu, you mean to calculate the
available number of frames here, so get_readable_frames() and
get_writable_frames() could be possible function names.


>   */
>  guint
>  read_space()
>  {
>    int wpos = Atomic::int_get (&write_frame_pos);
>    int rpos = Atomic::int_get (&read_frame_pos);
>    int size = buffer.size() / elements_per_frame;
>
>    if (wpos < rpos)		    /* wpos == rpos -> empty ringbuffer */
>      wpos += size;

if just wpos == rpos means the ringbuffer is empty, how do you
distinguish that from a full ring buffer then?

>    return wpos - rpos;
>  }
>  /**
>   * reads data from the ringbuffer
>   *
>   * @returns the number of successfully read frames
>   */
>  guint
>  read (guint n_frames, T* frames)
>  {
>    int rpos = Atomic::int_get (&read_frame_pos);
>    guint size = buffer.size() / elements_per_frame;
>    guint can_read = min (read_space(), n_frames);
>
>    BufferIterator start = buffer.begin() + rpos * elements_per_frame;
>    guint read1 = min (can_read, size - rpos) * elements_per_frame;
>    copy (start, start + read1, frames);

you should use bseblockuitls.h functions to copy floats.

>    guint read2 = can_read * elements_per_frame - read1;
>    copy (buffer.begin(), buffer.begin() + read2, frames + read1);
>
>    Atomic::int_set (&read_frame_pos, (rpos + can_read) % size);
>    return can_read;
>  }
>  /**
>   * checks available write space in the ringbuffer
>   *
>   * @returns the number of frames that can be written
>   */
>  guint
>  write_space()
>  {
>    int wpos = Atomic::int_get (&write_frame_pos);
>    int rpos = Atomic::int_get (&read_frame_pos);
>    int size = buffer.size() / elements_per_frame;
>
>    if (rpos <= wpos)		    /* wpos == rpos -> empty ringbuffer */
>      rpos += size;
>
>    /* the extra element allows us to see the difference between
>     *  - empty ringbuffer
>     *  - full ringbuffer
>     */
>    return rpos - wpos - 1;
>  }
>  /**
>   * writes data to the ringbuffer
>   *
>   * @returns the number of successfully written frames
>   */
>  guint
>  write (guint n_frames, const T* frames)
>  {
>    int wpos = Atomic::int_get (&write_frame_pos);
>    guint size = buffer.size() / elements_per_frame;
>    guint can_write = min (write_space(), n_frames);
>
>    BufferIterator start = buffer.begin() + wpos * elements_per_frame;
>    guint write1 = min (can_write, size - wpos) * elements_per_frame;
>    copy (frames, frames + write1, start);
>
>    guint write2 = can_write * elements_per_frame - write1;
>    copy (frames + write1, frames + write1 + write2, buffer.begin());
>
>    Atomic::int_set (&write_frame_pos, (wpos + can_write) % size);
>    return can_write;
>  }
>  /**
>   * returns the maximum number of frames that the ringbuffer can contain
>   */
>  guint
>  size() const
>  {
>    return (buffer.size() - 1) / elements_per_frame;
>  }
>  /**
>   * clears the ringbuffer
>   * this function is not! threadsafe

in general, if you mean to put docuemtnation comments next to your functions,
please use regular sentences and punctuation. emphasis can be added with 
@emph{}, not an exclamation mark in the middle of a sentence.
also, functions that take arguments and return values need extra docs, like:
 * @param file_pattern  wildcard pattern for file names
 * @return              a singly linked list with newly allocated strings


>   */
>  void
>  clear()
>  {
>    Atomic::int_set (&read_frame_pos, 0);
>    Atomic::int_set (&write_frame_pos, 0);
>  }
>  /**
>   * resizes and clears the ringbuffer
>   * this function is not! threadsafe
>   */
>  void
>  resize (guint n_frames,
>          guint elements_per_frame = 1)
>  {
>    this->elements_per_frame = elements_per_frame;
>    buffer.resize ((n_frames + 1) * elements_per_frame);
>    clear();
>  }
> };
>
>   Cu... Stefan
> --
> Stefan Westerfeld, Hamburg/Germany, http://space.twc.de/~stefan

---
ciaoTJ