search

jackaudio / jack2

jack2 codebase
GNU General Public License v2.0
2.22k stars 377 forks source link

Why does `jack_ringbuffer_reset()` `memset` the memory? #373

Open giuliomoro opened 6 years ago

giuliomoro commented 6 years ago

At this line https://github.com/jackaudio/jack2/blob/21f67b38df792d9226a892cbe853cd28832a96c1/common/ringbuffer.c#L129

If the write / read pointer are reset, this effectively makes the ringbuffer empty, as far as the user is concerned. There should therefore be no reason for memsetting the memory to 0. Also, neither jack_ringbuffer_create() nor jack_ringbuffer_reset_size() do memset the memory, so I think it should go from here as well. As it is, this can be a performance killer.

7890 commented 6 years ago

This is debatable, I see your point. My feeling is that jack_ringbuffer_reset() isn't called periodically or at a high rate. The performance gain would possibly be very small.

giuliomoro commented 6 years ago

When I stumbled upon this issue it was because I was getting occasional dropouts because third-party code was doing exactly that. Say that you are reading from disk into a pre-allocated buffer, you stop reading from disk, you want the buffer to be ready for reading a new file as soon as possible, with minimum overhead. I see no reason why jack_ringbuffer_reset() should differ from jack_ringbuffer_reset_size() and jack_ringbuffer_create() in this. The change would be not breaking: existing users would not be affected (except by the improved performance).

7890 commented 5 years ago

Stupidly asked, why does thrid-party code not use jack_ringbuffer_reset_size()? IIUC it would do just that, set the r/w pointers to make an empty buffer and leave the data to be overwritten on the next occasion.

The problem when changing the behaviour of jack_ringbuffer_reset() is that some clients might depend and expect the current behaviour, but that's just speculation.

Adding a note to the jack_ringbuffer_reset() API documentation mentioning it will memset would indeed be a good addition.

giuliomoro commented 5 years ago

Stupidly asked, why does thrid-party code not use jack_ringbuffer_reset_size()? IIUC it would do just that, set the r/w pointers to make an empty buffer and leave the data to be overwritten on the next occasion.

That's exactly what I ended up doing, but in order to find out that the problem was with jack_ringbuffer_reset() and that the workaround was jack_ringbuffer_reset_size(), I had to read the source code in ringbuffer.c. So, sure adding a note in the documentation will help, however I think that having two methods jack_ringbuffer_reset_size() and jack_ringbuffer_reset() whose functionalities differ for more than the changing the _size() violates the principle of least surprise. Furthermore, IIUC, there is no way for the user to get access to the content of any portion of the underlying char* buf that has not been explicitly written to by the user after a reset(), so if they are accessing the contents of the struct jack_ringbuffer_t directly and not through the API, they should accept that their code can break at any time?

On the other hand, I completely understand that making such change to a file that has not changed in 10 years, could bear some risks, especially as it's part of such a widely adopted codebase.