[vdr] [PATCH] UnbufferedFile improvements v7

[Klaus Schmidinger]

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