Re: [vdr] [PATCH] UnbufferedFile improvements v7

Artur Skawina wrote:

Klaus Schmidinger wrote:

...

Artur Skawina wrote:

...

this time with a new approach to read caching. Should make watching and editing recordings on a non-idle (and/or slow) machine more comfortable.

The difference to previous versions (and stock fadvise-enabled vdr) is that previously, read data was almost immediately forgotten after it was used; now a certain amount of recently accessed data (at most ~16M) is kept around. This means that short seeks (jumps) when replaying do not cause disk accesses. Things like switching play mode, FF, setting and moving editing marks shouldn't usually block waiting for disk IO. The changes are most noticeable when eg. several recordings are happening in the background.

I did very little testing, treat this as beta quality at best. Seems to work ok, but i won't probably have time to test it further for a few days; maybe somebody wants to play w/ this, or even better take a look at the Read() path...

after running w/ it for a week, i'd now say it works; haven't seen any problems that could be related to the Read() code.

...

Attached is the patch as I would add it for the next version.

Why not the inlined Seeks()? The IO is very inefficient anyway (tiny read/write sizes), having an extra syscall (seek) for every read/write call doesn't help... (yes, it may not be pretty, but it lets the compiler optimize the calls away, w/o touching any callers. Unless you fixed the callers, then ignore this)

Do you really think that making Seek() inline actually makes things faster? Can you show some hard numbers?

...

Please take a look at the lines marked CLARIFY and let me know if you have any comment on these.

i'll have to rediff this and compare w/ my current version; will post merged version later. Comments below.

...

+++ cutter.c 2006/02/04 13:40:20 @@ -66,6 +66,8 @@ toFile = toFileName->Open(); if (!fromFile || !toFile) return;

• fromFile->SetReadAhead(MEGABYTE(20));
  

• toFile->SetReadAhead(MEGABYTE(20));
  int Index = Mark->position;
  Mark = fromMarks.Next(Mark);
  int FileSize = 0;
  

@@ -122,6 +125,7 @@ error = "toFile 1"; break; }

•             toFile->SetReadAhead(MEGABYTE(20));
              FileSize = 0;
              }
           LastIFrame = 0;
  

@@ -162,6 +166,7 @@ error = "toFile 2"; break; }

•                toFile->SetReadAhead(MEGABYTE(20));
                 FileSize = 0;
                 }
              }
  

i've dropped the toFile->SetReadAhead() hack -- it doesn't change that much when the datasyncs aren't there anyway.

...

+++ tools.c 2006/02/04 13:55:24

•    // Read ahead:
  

•    // no jump? (allow small forward jump still inside readahead 
  

window).

•    if (jumped >= 0 && jumped <= (off_t)readahead) {
  

•       // Trigger the readahead IO, but only if we've used at least
  

•       // 1/2 of the previously requested area. This avoids calling
  

•       // fadvise() after every read() call.
  

•       if (ahead - curpos < (off_t)(readahead - readahead / 2)) 
  

{//XXX CLARIFY: isn't this just 'readahead / 2'???

historical reasons -- it used to be 1/4, not 1/2. I went back to 1/2 after switching to the new read caching strategy; previously i was seeing stalls when replaying and eg cutting at the same time. With the new code hopefully 1/2 is enough and reduces the number of fadvise calls.

So we could make this

if (ahead - curpos < (off_t)(readahead / 2)) {

then?

...

•       // The above fadvise() works when writing slowly 
  

(recording), but could

•       // leave cached data around when writing at a high rate 
  

(cutting).

•       // Also, it seems in some setups, the above does not 
  

trigger any I/O and

•       // the fdatasync() call below has to do all the work 
  

(reiserfs with some

•       // kind of write gathering enabled).
  

•       // We add 'readahead' to the threshold in an attempt to 
  

increase cutting

•       // speed; it's a tradeoff -- speed vs RAM-used.
  

•       if (totwritten > MEGABYTE(32) + readahead) {
  

•          //fdatasync(fd);//XXX CLARIFY: so what, is this 
  

necessary or not???

it is not, at least not here, however i left it commented out in case there are setups where it actually helps -- it's easier to uncomment this one line for testing than to find the right place to insert the call. It's not on by default because fsyncs are relatively expensive; i'd rather avoid them unless absolutely required. (i've updated the comments in this area since, they will be in the merged version)

Ok. Please make that a diff against version 1.3.41 with vdr-1.3.41-fadvise.diff applied.

[for faster responses please cc me; esp. if it's a reply to an older thread -- i almost missed this]

Don't worry, your response was fast enough ;-)

Klaus