[vlc-devel] [PATCH 1/6] demux/adaptive: Downloader: faster response to request
Zhao Zhili
quinkblack at foxmail.com
Thu Sep 21 03:46:24 CEST 2017
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.
More information about the vlc-devel
mailing list