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

[Rémi Denis-Courmont]

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.


> @@ -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/