[vlc-devel] [PATCH 1/6] demux/adaptive: Downloader: faster response to request

[Zhao Zhili]

> On 20 Sep 2017, at 11:37 PM, Rémi Denis-Courmont <remi at remlab.net> wrote:
> 
> Le keskiviikkona 20. syyskuuta 2017, 11.48.30 EEST Zhao Zhili a écrit :
>> ---
>>  modules/demux/adaptive/http/Downloader.cpp | 21 +++++++++++++++------
>>  modules/demux/adaptive/http/Downloader.hpp |  4 +++-
>>  2 files changed, 18 insertions(+), 7 deletions(-)
>> 
>> diff --git a/modules/demux/adaptive/http/Downloader.cpp
>> b/modules/demux/adaptive/http/Downloader.cpp
>> index ba81727..b2e64ef 100644
>> --- a/modules/demux/adaptive/http/Downloader.cpp
>> +++ b/modules/demux/adaptive/http/Downloader.cpp
>> @@ -28,12 +28,13 @@
>> 
>>  using namespace adaptive::http;
>> 
>> -Downloader::Downloader()
>> +Downloader::Downloader() :
>> +    thread_handle_valid(false),
>> +    killed(false),
>> +    pending_requests(0)
>>  {
>>      vlc_mutex_init(&lock);
>>      vlc_cond_init(&waitcond);
>> -    killed = false;
>> -    thread_handle_valid = false;
>>  }
>> 
>>  bool Downloader::start()
>> @@ -50,8 +51,8 @@ bool Downloader::start()
>> 
>>  Downloader::~Downloader()
>>  {
>> +    killed.store(true);
>>      vlc_mutex_lock( &lock );
>> -    killed = true;
>>      vlc_cond_signal(&waitcond);
>>      vlc_mutex_unlock( &lock );
> 
> This is an anti-patter and therefore a _very_ bad example that I don´t want to 
> see in the VLC code base.
> 
> I suppose that it works because of CSQ order on the atomic, but it´s still not 
> something I want to see cargo-culted.
> 
> Besides, using atomic variables here is actually _slower_ not faster. It only 
> adds extra memory ordering burden on the compiler, with no actual gain since 
> the mutex is still there.
> 

I don’t get it. The Downloader thread hold a lock and doesn’t response to request
until chunks.empty(). It’s faster because we don’t need to wait a whole chunk (maybe
a few megabytes) been downloaded before acquire the lock.

> 
>> @@ -62,18 +63,23 @@ Downloader::~Downloader()
>>  }
>>  void Downloader::schedule(HTTPChunkBufferedSource *source)
>>  {
>> +    pending_requests.fetch_add(1);
>>      vlc_mutex_lock(&lock);
>>      source->hold();
>>      chunks.push_back(source);
>> +    pending_requests.fetch_sub(1);
>>      vlc_cond_signal(&waitcond);
>>      vlc_mutex_unlock(&lock);
>>  }
>> 
>>  void Downloader::cancel(HTTPChunkBufferedSource *source)
>>  {
>> +    pending_requests.fetch_add(1);
>>      vlc_mutex_lock(&lock);
>>      source->release();
>>      chunks.remove(source);
>> +    pending_requests.fetch_sub(1);
>> +    vlc_cond_signal(&waitcond);
>>      vlc_mutex_unlock(&lock);
>>  }
>> 
>> @@ -97,10 +103,13 @@ void Downloader::Run()
>>      vlc_mutex_lock(&lock);
>>      while(1)
>>      {
>> -        while(chunks.empty() && !killed)
>> +        while(chunks.empty() && killed.load() == false)
>> +            vlc_cond_wait(&waitcond, &lock);
>> +
>> +        while(pending_requests.load() > 0 && killed.load() == false)
>>              vlc_cond_wait(&waitcond, &lock);
>> 
>> -        if(killed)
>> +        if(killed.load())
>>              break;
>> 
>>          if(!chunks.empty())
>> diff --git a/modules/demux/adaptive/http/Downloader.hpp
>> b/modules/demux/adaptive/http/Downloader.hpp
>> index 1d6cc57..ec76b3f 100644
>> --- a/modules/demux/adaptive/http/Downloader.hpp
>> +++ b/modules/demux/adaptive/http/Downloader.hpp
>> @@ -23,6 +23,7 @@
>>  #include "Chunk.h"
>> 
>>  #include <vlc_common.h>
>> +#include <atomic>
>>  #include <list>
>> 
>>  namespace adaptive
>> @@ -48,7 +49,8 @@ namespace adaptive
>>                  vlc_mutex_t  lock;
>>                  vlc_cond_t   waitcond;
>>                  bool         thread_handle_valid;
>> -                bool         killed;
>> +                std::atomic_bool killed;
>> +                std::atomic_int pending_requests;
>>                  std::list<HTTPChunkBufferedSource *> chunks;
>>          };
> 
> 
> -- 
> 雷米‧德尼-库尔蒙
> https://www.remlab.net/
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel