diff mbox

[FFmpeg-devel] Avoid creating unecessary dependencies on thread libraries.

Message ID 1479740313-25080-1-git-send-email-gregory.wolfe@kodakalaris.com
State Superseded
Headers show

Commit Message

Gregory J. Wolfe Nov. 21, 2016, 2:58 p.m. UTC 
(1) Multi-threading support requires knowing the number of CPUs available.
When building with MinGW on a Windows system, both gcc and Windows run
time functions are available to get this information.  However, when Windows
threading has been selected, the Windows function should be used, not the
gcc function.  This avoids creating an unnecessary dependency on the gcc
thread library.

(2) When ALL threading support is disabled, the build should not create a
dependency on ANY thread library.

Signed-off-by: Gregory J. Wolfe <gregory.wolfe@kodakalaris.com>
---
 libavutil/cpu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

wm4 Nov. 21, 2016, 3:05 p.m. UTC | #1 
On Mon, 21 Nov 2016 09:58:33 -0500
"Gregory J. Wolfe" <gregory.wolfe@kodakalaris.com> wrote:

> (1) Multi-threading support requires knowing the number of CPUs available.
> When building with MinGW on a Windows system, both gcc and Windows run
> time functions are available to get this information.  However, when Windows
> threading has been selected, the Windows function should be used, not the
> gcc function.  This avoids creating an unnecessary dependency on the gcc
> thread library.
> 
> (2) When ALL threading support is disabled, the build should not create a
> dependency on ANY thread library.
> 
> Signed-off-by: Gregory J. Wolfe <gregory.wolfe@kodakalaris.com>
> ---
>  libavutil/cpu.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
> index f5785fc..3843778 100644
> --- a/libavutil/cpu.c
> +++ b/libavutil/cpu.c
> @@ -258,10 +258,15 @@ int av_cpu_count(void)
>      static volatile int printed;
>  
>      int nb_cpus = 1;
> +#if HAVE_THREADS
>  #if HAVE_WINRT
>      SYSTEM_INFO sysinfo;
>  #endif
> -#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT)
> +    // if HAVE_W32THREADS and HAVE_GETPROCESSAFFINITYMASK, we will use
> +    // Windows GetProcessAffinityMask() instead of gcc library function
> +    // sched_getaffinity().  This avoids creating a dependency on the gcc
> +    // thread library that we don't need/want.
> +#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT) && !(HAVE_W32THREADS && HAVE_GETPROCESSAFFINITYMASK)
>      cpu_set_t cpuset;
>  
>      CPU_ZERO(&cpuset);
> @@ -286,6 +291,7 @@ int av_cpu_count(void)
>      GetNativeSystemInfo(&sysinfo);
>      nb_cpus = sysinfo.dwNumberOfProcessors;
>  #endif
> +#endif
>  
>      if (!printed) {
>          av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n", nb_cpus);

Wouldn't it be better to move the HAVE_GETPROCESSAFFINITYMASK case
above HAVE_SCHED_GETAFFINITY, and avoid making the ifdeffery mess
worse? GetProcessAffinityMask is available on every supported Windows
and could as well be replaced by "ifdef _WIN32" instead.

Also, technically you might have to stop the configure script to stop
linking to libpthread. I'm not sure what exactly stops it from doing
that in your case.
Gregory J. Wolfe Nov. 21, 2016, 4:23 p.m. UTC | #2 
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf Of wm4

> Sent: Monday, November 21, 2016 10:05 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH] Avoid creating unecessary

> dependencies on thread libraries.

> 

> On Mon, 21 Nov 2016 09:58:33 -0500

> "Gregory J. Wolfe" <gregory.wolfe@kodakalaris.com> wrote:

> 

> > (1) Multi-threading support requires knowing the number of CPUs

> available.

> > When building with MinGW on a Windows system, both gcc and

> Windows run

> > time functions are available to get this information.  However, when

> Windows

> > threading has been selected, the Windows function should be used,

> not the

> > gcc function.  This avoids creating an unnecessary dependency on

> the gcc

> > thread library.

> >

> > (2) When ALL threading support is disabled, the build should not

> create a

> > dependency on ANY thread library.

> >

> > Signed-off-by: Gregory J. Wolfe <gregory.wolfe@kodakalaris.com>

> > ---

> >  libavutil/cpu.c | 8 +++++++-

> >  1 file changed, 7 insertions(+), 1 deletion(-)

> >

> > diff --git a/libavutil/cpu.c b/libavutil/cpu.c

> > index f5785fc..3843778 100644

> > --- a/libavutil/cpu.c

> > +++ b/libavutil/cpu.c

> > @@ -258,10 +258,15 @@ int av_cpu_count(void)

> >      static volatile int printed;

> >

> >      int nb_cpus = 1;

> > +#if HAVE_THREADS

> >  #if HAVE_WINRT

> >      SYSTEM_INFO sysinfo;

> >  #endif

> > -#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT)

> > +    // if HAVE_W32THREADS and HAVE_GETPROCESSAFFINITYMASK,

> we will use

> > +    // Windows GetProcessAffinityMask() instead of gcc library

> function

> > +    // sched_getaffinity().  This avoids creating a dependency on the

> gcc

> > +    // thread library that we don't need/want.

> > +#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT) &&

> !(HAVE_W32THREADS && HAVE_GETPROCESSAFFINITYMASK)

> >      cpu_set_t cpuset;

> >

> >      CPU_ZERO(&cpuset);

> > @@ -286,6 +291,7 @@ int av_cpu_count(void)

> >      GetNativeSystemInfo(&sysinfo);

> >      nb_cpus = sysinfo.dwNumberOfProcessors;

> >  #endif

> > +#endif

> >

> >      if (!printed) {

> >          av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n",

> nb_cpus);

> 

> Wouldn't it be better to move the HAVE_GETPROCESSAFFINITYMASK

> case

> above HAVE_SCHED_GETAFFINITY, and avoid making the ifdeffery

> mess

> worse? GetProcessAffinityMask is available on every supported

> Windows

> and could as well be replaced by "ifdef _WIN32" instead.

> 


Yeah, I had the same thought.  For the first go around I opted for the
solution that moved around the least code, not knowing for sure if
I might cause some undesirable side effects by reordering the existing
code.  I can easily redo the patch if we're in agreement on this point.

> Also, technically you might have to stop the configure script to stop

> linking to libpthread. I'm not sure what exactly stops it from doing

> that in your case.

 
The result I observed was that the resulting avutil.dll did NOT have
a dependency on the gcc run time.  My interpretation of this is that
if you link against a library, but don't reference any symbols in it, then
it's as if you never linked against it in the first place.

Greg W.
wm4 Nov. 21, 2016, 4:34 p.m. UTC | #3 
On Mon, 21 Nov 2016 16:23:03 +0000
Gregory J Wolfe <gregory.wolfe@kodakalaris.com> wrote:

> > -----Original Message-----
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On
> > Behalf Of wm4
> > Sent: Monday, November 21, 2016 10:05 AM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH] Avoid creating unecessary
> > dependencies on thread libraries.
> > 
> > On Mon, 21 Nov 2016 09:58:33 -0500
> > "Gregory J. Wolfe" <gregory.wolfe@kodakalaris.com> wrote:
> >   
> > > (1) Multi-threading support requires knowing the number of CPUs  
> > available.  
> > > When building with MinGW on a Windows system, both gcc and  
> > Windows run  
> > > time functions are available to get this information.  However, when  
> > Windows  
> > > threading has been selected, the Windows function should be used,  
> > not the  
> > > gcc function.  This avoids creating an unnecessary dependency on  
> > the gcc  
> > > thread library.
> > >
> > > (2) When ALL threading support is disabled, the build should not  
> > create a  
> > > dependency on ANY thread library.
> > >
> > > Signed-off-by: Gregory J. Wolfe <gregory.wolfe@kodakalaris.com>
> > > ---
> > >  libavutil/cpu.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavutil/cpu.c b/libavutil/cpu.c
> > > index f5785fc..3843778 100644
> > > --- a/libavutil/cpu.c
> > > +++ b/libavutil/cpu.c
> > > @@ -258,10 +258,15 @@ int av_cpu_count(void)
> > >      static volatile int printed;
> > >
> > >      int nb_cpus = 1;
> > > +#if HAVE_THREADS
> > >  #if HAVE_WINRT
> > >      SYSTEM_INFO sysinfo;
> > >  #endif
> > > -#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT)
> > > +    // if HAVE_W32THREADS and HAVE_GETPROCESSAFFINITYMASK,  
> > we will use  
> > > +    // Windows GetProcessAffinityMask() instead of gcc library  
> > function  
> > > +    // sched_getaffinity().  This avoids creating a dependency on the  
> > gcc  
> > > +    // thread library that we don't need/want.
> > > +#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT) &&  
> > !(HAVE_W32THREADS && HAVE_GETPROCESSAFFINITYMASK)  
> > >      cpu_set_t cpuset;
> > >
> > >      CPU_ZERO(&cpuset);
> > > @@ -286,6 +291,7 @@ int av_cpu_count(void)
> > >      GetNativeSystemInfo(&sysinfo);
> > >      nb_cpus = sysinfo.dwNumberOfProcessors;
> > >  #endif
> > > +#endif
> > >
> > >      if (!printed) {
> > >          av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n",  
> > nb_cpus);
> > 
> > Wouldn't it be better to move the HAVE_GETPROCESSAFFINITYMASK
> > case
> > above HAVE_SCHED_GETAFFINITY, and avoid making the ifdeffery
> > mess
> > worse? GetProcessAffinityMask is available on every supported
> > Windows
> > and could as well be replaced by "ifdef _WIN32" instead.
> >   
> 
> Yeah, I had the same thought.  For the first go around I opted for the
> solution that moved around the least code, not knowing for sure if
> I might cause some undesirable side effects by reordering the existing
> code.  I can easily redo the patch if we're in agreement on this point.

Depends if there are any cargo cult people who are against it.

> > Also, technically you might have to stop the configure script to stop
> > linking to libpthread. I'm not sure what exactly stops it from doing
> > that in your case.  
>  
> The result I observed was that the resulting avutil.dll did NOT have
> a dependency on the gcc run time.  My interpretation of this is that
> if you link against a library, but don't reference any symbols in it, then
> it's as if you never linked against it in the first place.

This could still be caused by certain link flag (forgot which one). In
general, linking against a dynamic library without using any of its
functions, could still have side effects, such as registering plugins
in the DLL init functions and such.
Hendrik Leppkes Nov. 21, 2016, 4:55 p.m. UTC | #4 
On Mon, Nov 21, 2016 at 5:23 PM, Gregory J Wolfe
<gregory.wolfe@kodakalaris.com> wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On
>> Behalf Of wm4
>> Sent: Monday, November 21, 2016 10:05 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH] Avoid creating unecessary
>> dependencies on thread libraries.
>>
>> On Mon, 21 Nov 2016 09:58:33 -0500
>> "Gregory J. Wolfe" <gregory.wolfe@kodakalaris.com> wrote:
>>
>> > (1) Multi-threading support requires knowing the number of CPUs
>> available.
>> > When building with MinGW on a Windows system, both gcc and
>> Windows run
>> > time functions are available to get this information.  However, when
>> Windows
>> > threading has been selected, the Windows function should be used,
>> not the
>> > gcc function.  This avoids creating an unnecessary dependency on
>> the gcc
>> > thread library.
>> >
>> > (2) When ALL threading support is disabled, the build should not
>> create a
>> > dependency on ANY thread library.
>> >
>> > Signed-off-by: Gregory J. Wolfe <gregory.wolfe@kodakalaris.com>
>> > ---
>> >  libavutil/cpu.c | 8 +++++++-
>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/libavutil/cpu.c b/libavutil/cpu.c
>> > index f5785fc..3843778 100644
>> > --- a/libavutil/cpu.c
>> > +++ b/libavutil/cpu.c
>> > @@ -258,10 +258,15 @@ int av_cpu_count(void)
>> >      static volatile int printed;
>> >
>> >      int nb_cpus = 1;
>> > +#if HAVE_THREADS
>> >  #if HAVE_WINRT
>> >      SYSTEM_INFO sysinfo;
>> >  #endif
>> > -#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT)
>> > +    // if HAVE_W32THREADS and HAVE_GETPROCESSAFFINITYMASK,
>> we will use
>> > +    // Windows GetProcessAffinityMask() instead of gcc library
>> function
>> > +    // sched_getaffinity().  This avoids creating a dependency on the
>> gcc
>> > +    // thread library that we don't need/want.
>> > +#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT) &&
>> !(HAVE_W32THREADS && HAVE_GETPROCESSAFFINITYMASK)
>> >      cpu_set_t cpuset;
>> >
>> >      CPU_ZERO(&cpuset);
>> > @@ -286,6 +291,7 @@ int av_cpu_count(void)
>> >      GetNativeSystemInfo(&sysinfo);
>> >      nb_cpus = sysinfo.dwNumberOfProcessors;
>> >  #endif
>> > +#endif
>> >
>> >      if (!printed) {
>> >          av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n",
>> nb_cpus);
>>
>> Wouldn't it be better to move the HAVE_GETPROCESSAFFINITYMASK
>> case
>> above HAVE_SCHED_GETAFFINITY, and avoid making the ifdeffery
>> mess
>> worse? GetProcessAffinityMask is available on every supported
>> Windows
>> and could as well be replaced by "ifdef _WIN32" instead.
>>
>
> Yeah, I had the same thought.  For the first go around I opted for the
> solution that moved around the least code, not knowing for sure if
> I might cause some undesirable side effects by reordering the existing
> code.  I can easily redo the patch if we're in agreement on this point.
>

Moving the code to prefer the win32 functions if available is
definitely preferable to further complicating the ifdefs to not only
check for features but also doing negative checks on other features.
The preferred option should just be on top - which in this case is the
OS-native function over the pthread functions, which are always some
sort of emulation layer ontop of the win32 functions anyway.

- Hendrik
diff mbox

Patch

diff --git a/libavutil/cpu.c b/libavutil/cpu.c
index f5785fc..3843778 100644
--- a/libavutil/cpu.c
+++ b/libavutil/cpu.c
@@ -258,10 +258,15 @@  int av_cpu_count(void)
     static volatile int printed;
 
     int nb_cpus = 1;
+#if HAVE_THREADS
 #if HAVE_WINRT
     SYSTEM_INFO sysinfo;
 #endif
-#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT)
+    // if HAVE_W32THREADS and HAVE_GETPROCESSAFFINITYMASK, we will use
+    // Windows GetProcessAffinityMask() instead of gcc library function
+    // sched_getaffinity().  This avoids creating a dependency on the gcc
+    // thread library that we don't need/want.
+#if HAVE_SCHED_GETAFFINITY && defined(CPU_COUNT) && !(HAVE_W32THREADS && HAVE_GETPROCESSAFFINITYMASK)
     cpu_set_t cpuset;
 
     CPU_ZERO(&cpuset);
@@ -286,6 +291,7 @@  int av_cpu_count(void)
     GetNativeSystemInfo(&sysinfo);
     nb_cpus = sysinfo.dwNumberOfProcessors;
 #endif
+#endif
 
     if (!printed) {
         av_log(NULL, AV_LOG_DEBUG, "detected %d logical cores\n", nb_cpus);