dynamically sized ringbuffers v2
Commit Message
Auto sized ringbuffers, changes since v1:
- increased maximum sizes for a few rb users. Most of the time just a
small part will be used, but there will be more room for times when
more is required.
- a little smarter cRingBufferLinear::Read(), handles the TS buffer
better.
- faster buffer growth. Also makes it less likely that there won't be
an opportunity to resize before an overflow.
- a potentially sleeping cRecorder:Receive(). Mostly to find out how
much more buffer/time would have been needed, ie debugging.
- more verbose logging.
It still wouldn't surprise me if this version caused a few overflows,
but hopefully these will be very rare.
artur
Comments
On 5/10/07, Artur Skawina <art_k@o2.pl> wrote:
>
> Auto sized ringbuffers, changes since v1:
>
> - increased maximum sizes for a few rb users. Most of the time just a
> small part will be used, but there will be more room for times when
> more is required.
> - a little smarter cRingBufferLinear::Read(), handles the TS buffer
> better.
> - faster buffer growth. Also makes it less likely that there won't be
> an opportunity to resize before an overflow.
> - a potentially sleeping cRecorder:Receive(). Mostly to find out how
> much more buffer/time would have been needed, ie debugging.
> - more verbose logging.
>
> It still wouldn't surprise me if this version caused a few overflows,
> but hopefully these will be very rare.
I'm curious how streamdev will function with these buffer changes.
Best Regards.
Stone writes:
> >
> > It still wouldn't surprise me if this version caused a few overflows,
> > but hopefully these will be very rare.
>
> I'm curious how streamdev will function with these buffer changes.
And since I am not convinced that this memory footprint issue is
significant, I am concerned if this patch is accepted before all the
overflows and problems with live viewing have been solved first.
yours,
Jouni
> And since I am not convinced that this memory footprint issue is
> significant,
at a first glance, IMHO dynamic buffers are a good thing. we can get
rid of small upper buffer size bounderies all together without wasting
amounts of memory. this should result in even less buffer overflows
when implemented correctly. it seems doable to me.
clemens
Stone wrote:
>
> It still wouldn't surprise me if this version caused a few overflows,
> but hopefully these will be very rare.
>
> I'm curious how streamdev will function with these buffer changes.
it works fine -- i'm using a headless vdr server and streamdev+softdevice clients, so
this actually gets tested fairly well.
Example buffer usage from server:
Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size)
Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size)
Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size)
Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size)
Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size)
Deleting ring buffer "streamdev-streamer" size: 524288 / 4194304 (used 12.5% of requested size)
Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size)
Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size)
Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size)
Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size)
Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size)
Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size)
Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size)
Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size)
Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size)
Deleting ring buffer "streamdev-streamer" size: 524288 / 4194304 (used 12.5% of requested size)
Deleting ring buffer "streamdev-streamer" size: 524288 / 4194304 (used 12.5% of requested size)
Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size)
Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size)
Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size)
Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size)
Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size)
Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size)
and the streamdev client TS buffer usually grows to 12.5..25% (ie 256K/512K out of 4M max).
artur
Jouni Karvo wrote:
> Stone writes:
> > >
> > > It still wouldn't surprise me if this version caused a few overflows,
> > > but hopefully these will be very rare.
> >
> > I'm curious how streamdev will function with these buffer changes.
>
> And since I am not convinced that this memory footprint issue is
> significant, I am concerned if this patch is accepted before all the
> overflows and problems with live viewing have been solved first.
It shouldn't. The whole point of posting it is so that other are able
to test and review it. That is why I always mention the overflows.
Right now it's stable enough for production for me, haven't seen any
overflow since adjusting the growth rate, but it would be nice if
somebody with a different vdr setup tested it too (eg a FF card based
vdr).
It will take weeks to verify that the code works as expected (the goal
is to have _fewer_ overflows (compared to the static buffers) while at
the same time a much smaller working set).
artur
On 05/10/07 01:55, Artur Skawina wrote:
> Auto sized ringbuffers, changes since v1:
> ...
> diff --git a/recorder.c b/recorder.c
> index 8bb1621..3c0e002 100644
> --- a/recorder.c
> +++ b/recorder.c
> @@ -157,8 +157,20 @@ void cRecorder::Receive(uchar *Data, int Length)
> {
> if (Running()) {
> int p = ringBuffer->Put(Data, Length);
> - if (p != Length && Running())
> + if (p != Length && Running()) {
> + for (int ms=20; ms<1000; ms+=ms) {
> + cCondWait::SleepMs(ms);
> + if (!Running())
> + return;
> + int r = ringBuffer->Put(Data+p, Length-p);
> + p += r;
> + if (r)
> + dsyslog("saved extra %d bytes in recorder ring buffer after %d ms delay", r, ms);
> + if (p == Length || !Running())
> + return;
> + }
> ringBuffer->ReportOverflow(Length - p);
> + }
> }
> }
If this "auto sized ringbuffers" change (which, from what I can
see so far - haven't tried it myself - looks like a good idea)
is ever to make its way into the official VDR source, you'll need
to get rid of the above waiting. It says in receiver.h:
...the call must return as soon as possible, without any unnecessary delay.
Maybe I should change this to
...the call must return immediately.
Klaus
Klaus Schmidinger wrote:
> If this "auto sized ringbuffers" change (which, from what I can
> see so far - haven't tried it myself - looks like a good idea)
> is ever to make its way into the official VDR source, you'll need
> to get rid of the above waiting. It says in receiver.h:
>
> ...the call must return as soon as possible, without any unnecessary delay.
>
> Maybe I should change this to
>
> ...the call must return immediately.
or "must not sleep".
I did mention in the patch description, which you snipped, that this was just
a debugging tool. IOW that change can be dropped entirely, the condition that
makes it sleep has never triggered since I made the buffers grow faster anyway.
I'd be great if somebody with a FF card could test the patch, the code has
worked flawlessly on my vdr server, not a single overflow since v2.
However on the streamdev/softdevice client I had two transfer buffer overflows
(153 bytes each) just after switching to a new channel, while both machines
were under significant (io) load, cutting aot. I don't think this is reason
enough to introduce a minsize rb parameter, but if it also happens w/ a simple
FF setup, that would be an option. I don't want to raise the default minimum,
because 128k is often enough and i'd like to avoid shrinking, to keep things
simple.
The patch is for 1.4.6, but applies also, with a few harmless offsets, to 1.5.2.
artur
Artur Skawina <art_k@o2.pl> wrote:
> I'd be great if somebody with a FF card could test the patch, the
> code has worked flawlessly on my vdr server, not a single overflow
> since v2.
i currently test your patch on vdr-1.5.2 with one FF card. i have some
timers pending this afternoon, so stay tuned. ;-)
clemens
@@ -1184,7 +1184,7 @@ bool cDvbDevice::OpenDvr(void)
CloseDvr();
fd_dvr = DvbOpen(DEV_DVB_DVR, CardIndex(), O_RDONLY | O_NONBLOCK, true);
if (fd_dvr >= 0)
- tsBuffer = new cTSBuffer(fd_dvr, MEGABYTE(2), CardIndex() + 1);
+ tsBuffer = new cTSBuffer(fd_dvr, MEGABYTE(8), CardIndex() + 1);
return fd_dvr >= 0;
}
@@ -157,8 +157,20 @@ void cRecorder::Receive(uchar *Data, int Length)
{
if (Running()) {
int p = ringBuffer->Put(Data, Length);
- if (p != Length && Running())
+ if (p != Length && Running()) {
+ for (int ms=20; ms<1000; ms+=ms) {
+ cCondWait::SleepMs(ms);
+ if (!Running())
+ return;
+ int r = ringBuffer->Put(Data+p, Length-p);
+ p += r;
+ if (r)
+ dsyslog("saved extra %d bytes in recorder ring buffer after %d ms delay", r, ms);
+ if (p == Length || !Running())
+ return;
+ }
ringBuffer->ReportOverflow(Length - p);
+ }
}
}
@@ -1851,7 +1851,7 @@ void cTS2PES::ts_to_pes(const uint8_t *Buf) // don't need count (=188)
// --- cRemux ----------------------------------------------------------------
-#define RESULTBUFFERSIZE KILOBYTE(256)
+#define RESULTBUFFERSIZE MEGABYTE(2)
cRemux::cRemux(int VPid, const int *APids, const int *DPids, const int *SPids, bool ExitOnFailure)
{
@@ -151,13 +151,30 @@ void cRingBufferLinear::PrintDebugRBL(void)
}
#endif
+// cRingBufferLinear are dynamically sized, or at least we can pretend they are ;)
+// We treat 'Size' as the maximum size, but start with a small buffer, which can
+// grow later as it fills up. Memory is always allocated for the full buffer, but
+// the unused RAM portion remains untouched until (if at all) it is actually needed.
+// Note that we can not start with a larger than requested buffer because there are
+// ring buffer users that cause crashes then (eg softdevice mpegdecoder absolutely
+// needs 32/64k).
+
+// Startup size. 64k still causes overflows before buffer starts to grow, 128k doesn't.
+// The buffers grow to 200..500k anyway, so maybe increasing this a bit more would
+// make sense, but let's first see if it's really needed.
+// In fact 128k is on the low side, but let's try not going for 256k yet.
+#define DEFRBSIZE KILOBYTE(128)
+
cRingBufferLinear::cRingBufferLinear(int Size, int Margin, bool Statistics, const char *Description)
-:cRingBuffer(Size, Statistics)
+:cRingBuffer(min(Size,DEFRBSIZE), Statistics)
{
description = Description ? strdup(Description) : NULL;
tail = head = margin = Margin;
gotten = 0;
buffer = NULL;
+ maxsize = Size;
+ growbuf = 0;
+ dsyslog("New ring buffer \"%s\" size: %d margin: %d", description, Size, Margin );
if (Size > 1) { // 'Size - 1' must not be 0!
if (Margin <= Size / 2) {
buffer = MALLOC(uchar, Size);
@@ -183,6 +200,8 @@ cRingBufferLinear::~cRingBufferLinear()
#ifdef DEBUGRINGBUFFERS
DelDebugRBL(this);
#endif
+ dsyslog("Deleting ring buffer \"%s\" size: %d / %d (used %g%% of requested size)", description,
+ size, maxsize, size/(double)maxsize*100.0 );
free(buffer);
free(description);
}
@@ -205,8 +224,21 @@ void cRingBufferLinear::Clear(void)
EnablePut();
}
+// Must only be called by the producer (ie Read()/Put()), not the consumer (Get()).
+void cRingBufferLinear::GrowBuffer(void)
+{
+ size = min(size+size, maxsize);
+ dsyslog("Enlarging ring buffer \"%s\": %d bytes (trigger %d)",
+ description, size, growbuf);
+ growbuf = 0;
+}
+
int cRingBufferLinear::Read(int FileHandle, int Max)
{
+ if (Available() >= size/2)
+ growbuf = 1;
+ if (growbuf && head>=tail && size<maxsize)
+ GrowBuffer();
int Tail = tail;
int diff = Tail - head;
int free = (diff > 0) ? diff - 1 : Size() - head;
@@ -219,6 +251,10 @@ int cRingBufferLinear::Read(int FileHandle, int Max)
Count = safe_read(FileHandle, buffer + head, free);
if (Count > 0) {
int Head = head + Count;
+ if (Available()+Count >= size/2)
+ growbuf = 2;
+ if (growbuf && head>=tail && size<maxsize)
+ GrowBuffer();
if (Head >= Size())
Head = margin;
head = Head;
@@ -245,6 +281,10 @@ int cRingBufferLinear::Read(int FileHandle, int Max)
int cRingBufferLinear::Put(const uchar *Data, int Count)
{
if (Count > 0) {
+ if (Available()+Count >= size/2)
+ growbuf = 3;
+ if (growbuf && head>=tail && size<maxsize)
+ GrowBuffer();
int Tail = tail;
int rest = Size() - head;
int diff = Tail - head;
@@ -18,11 +18,11 @@ private:
cCondWait readyForPut, readyForGet;
int putTimeout;
int getTimeout;
- int size;
time_t lastOverflowReport;
int overflowCount;
int overflowBytes;
protected:
+ int size;
tThreadId getThreadTid;
int maxFill;//XXX
int lastPercent;
@@ -59,7 +59,10 @@ private:
int margin, head, tail;
int gotten;
uchar *buffer;
+ int growbuf;
+ int maxsize;
char *description;
+ void GrowBuffer(void);
public:
cRingBufferLinear(int Size, int Margin = 0, bool Statistics = false, const char *Description = NULL);
///< Creates a linear ring buffer.