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

[Zhao Zhili]

Hi Hugo,


On 2017年09月20日 17:01, Hugo Beauzée-Luyssen wrote:
> Hi,
>
> On Wed, Sep 20, 2017, at 05:48 AM, Zhao Zhili wrote:
>> ---
>>    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);
> Since the wait condition is always signaled/wait upon in a locked scope,
> I'm not sure this improves much

Call vlc_mutex_lock(&lock) and then set killed to true, 
vlc_mutex_lock(&lock) will block until chunks.empty() is true.

  98 while(1)
  99     {
100         while(chunks.empty() && !killed)
101             vlc_cond_wait(&waitcond, &lock);
102

If we make killed an atomic variable and set it to true first, the big 
while(1) in Downloader::Run() will break out more quickly since we don't 
need to wait chunks.empty().

>>        vlc_mutex_lock( &lock );
>> -    killed = true;
>>        vlc_cond_signal(&waitcond);
>>        vlc_mutex_unlock( &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)
> I'm not sure I understand the idea behind this second wait

It's the same idea as above, make Downloader::schedule() and
Downloader::cancel() acquire the lock more quickly.

>>                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;
>>            };
>>
>> -- 
>> 2.7.4
>>
> Regards,
>