17 Oct
2008
17 Oct
'08
4:36 p.m.
Fons Adriaensen wrote:
On Fri, Oct 17, 2008 at 08:06:55PM +0200, Joern
Nettingsmeier wrote:
I don't see why it's fragile. There's no reason for the compiler to modify
rb->read_ptr if not asked to. Buf if that's the case, because of some sort of
dark voodoo optimization, then we could use a temporary variable to hold the
intermediary result. Better, all computation could be done on this temporary
variable and transfered into rb->read_ptr once finished, so that modifying the
later only involves memory copy.
excuse my chiming in here, i'm not really
much of a c programmer... but
how can this
- rb->read_ptr += n1;
- rb->read_ptr &= rb->size_mask;
+ rb->read_ptr = (rb->read_ptr + n1) & rb->size_mask;
fix anything?
iiuc, both versions are equivalent. a context switch could happen just
as well after the parenthesis has been computed..!?
putting stuff on one line doesn't make it atomic. maybe you are now
getting another compiler optimization that helps to hide the bug?
The idea is that it is very unlikely that the compiler
would store the intermediate result in rb->read_ptr,
and so this value is updated only once.
I agree it is fragile, there is nothing stopping
the compiler from doing it wrong. Unless the
the pointers are made volatile again. One real solution is to *not* use the size_mask
at this point, but apply it only when read_ptr
and write_ptr are read. So only a single addition
remains. The pointers are allowed to grow, they
will wrap around at the word size, but since the
buffer size is a power of two they are still
correct when that happens.
This is what is done in the C++ class I
posted earlier.
You class looks good, but some work is needed to provide a Jack backward
compatible API based on it.
IMO, one important thing is to include unit/regression tests, for the ringbuffer
as well as general Jack operation, in the official jack distribution, that one
can run with a simple "make test". I'll try to do this for this test suite
I
wrote, once cleaned up.
--
Olivier Guilyardi / Samalyse