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

[Zhao Zhili]

On 2017年09月21日 00:16, Zhao Zhili wrote:
>> 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
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

A simple test case.

Server: Apache with bandwidth limited to 200KB/S
Stream: HLS VOD with average bitrate 170KB/S

Without this patch, close adaptive takes:

[00007f1d08013220] adaptive demux error: Close takes 3.941656 seconds
[00007f1ce00011b0] adaptive demux error: Close takes 4.052950 seconds
[00007f1cd56e1a20] adaptive demux error: Close takes 1.470584 seconds
[00007f1cd56e93f0] adaptive demux error: Close takes 3.286187 seconds
[00007f1cf80955e0] adaptive demux error: Close takes 0.000258 seconds
[00007f1d041a1b70] adaptive demux error: Close takes 4.253166 seconds
[00007f1cf1211c90] adaptive demux error: Close takes 4.140404 second

With this patch, close adaptive takes:

[00007fc08c008dc0] adaptive demux error: Close takes 0.209676 seconds
[00007fc08c000c20] adaptive demux error: Close takes 0.041744 seconds
[00007fc08c02c860] adaptive demux error: Close takes 0.191418 seconds
[00007fc05c006230] adaptive demux error: Close takes 0.169747 seconds
[00007fc0616f72f0] adaptive demux error: Close takes 0.254685 seconds
[00007fc0616f0450] adaptive demux error: Close takes 0.183926 seconds
[00007fc0745e78d0] adaptive demux error: Close takes 0.147681 seconds
[00007fc0880e4f40] adaptive demux error: Close takes 0.170514 seconds

With the group of patches, close adaptive takes:
[00007fb6a000baf0] adaptive demux error: Close takes 0.001740 seconds
[00007fb68c003120] adaptive demux error: Close takes 0.000438 seconds
[00007fb6696b4d00] adaptive demux error: Close takes 0.000646 seconds
[00007fb6841afd10] adaptive demux error: Close takes 0.000877 seconds
[00007fb668000b60] adaptive demux error: Close takes 0.003986 seconds
[00007fb6a40019a0] adaptive demux error: Close takes 0.001366 seconds
[00007fb6ac004250] adaptive demux error: Close takes 0.000552 seconds

It's not an elegant solution, but it works for me. I would be glad to know a
clean and lightweight solution for these cases.