[PATCH 1/3] debug.h: cleanup TRACE macros
The complex "#ifdef WINE_NO_TRACE_MSGS && __compiler__" ladder below reduces to "ignore WINE_NO_TRACE_MSGS when !__GNUC__ && !__SUNPRO_C", which is probably a bug. The other nuance is the removed "args" prefix, which is a GNU extension, and was not used anyway. Let's just define all of that in a single place. Signed-off-by: Konstantin Kharlamov <Hi-Angel(a)yandex.ru> --- include/wine/debug.h | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/include/wine/debug.h b/include/wine/debug.h index 07ac88d264..069e754dfe 100644 --- a/include/wine/debug.h +++ b/include/wine/debug.h @@ -59,8 +59,12 @@ struct __wine_debug_channel #ifndef WINE_NO_TRACE_MSGS # define __WINE_GET_DEBUGGING_TRACE(dbch) ((dbch)->flags & (1 << __WINE_DBCL_TRACE)) +# define WINE_TRACE __WINE_DPRINTF(_TRACE,__wine_dbch___default) +# define WINE_TRACE_(ch) __WINE_DPRINTF(_TRACE,&__wine_dbch_##ch) #else # define __WINE_GET_DEBUGGING_TRACE(dbch) 0 +# define WINE_TRACE(args...) do { } while(0) +# define WINE_TRACE_(ch) WINE_TRACE #endif #ifndef WINE_NO_DEBUG_MSGS @@ -92,12 +96,6 @@ struct __wine_debug_channel #define __WINE_PRINTF_ATTR(fmt,args) __attribute__((format (printf,fmt,args))) - -#ifdef WINE_NO_TRACE_MSGS -#define WINE_TRACE(args...) do { } while(0) -#define WINE_TRACE_(ch) WINE_TRACE -#endif - #ifdef WINE_NO_DEBUG_MSGS #define WINE_WARN(args...) do { } while(0) #define WINE_WARN_(ch) WINE_WARN @@ -118,11 +116,6 @@ struct __wine_debug_channel #define __WINE_PRINTF_ATTR(fmt,args) -#ifdef WINE_NO_TRACE_MSGS -#define WINE_TRACE(...) do { } while(0) -#define WINE_TRACE_(ch) WINE_TRACE -#endif - #ifdef WINE_NO_DEBUG_MSGS #define WINE_WARN(...) do { } while(0) #define WINE_WARN_(ch) WINE_WARN @@ -337,10 +330,6 @@ static inline const char *wine_dbgstr_variant( const VARIANT *v ) #endif /* defined(__oaidl_h__) && defined(V_VT) */ -#ifndef WINE_TRACE -#define WINE_TRACE __WINE_DPRINTF(_TRACE,__wine_dbch___default) -#define WINE_TRACE_(ch) __WINE_DPRINTF(_TRACE,&__wine_dbch_##ch) -#endif #define WINE_TRACE_ON(ch) __WINE_IS_DEBUG_ON(_TRACE,&__wine_dbch_##ch) #ifndef WINE_WARN -- 2.20.1
Same as prev. commit. The complex "#ifdef WINE_NO_DEBUG_MSGS && __compiler__" ladder below reduces to "ignore WINE_NO_DEBUG_MSGS when !__GNUC__ && !__SUNPRO_C", which is probably a bug. The other nuance is the removed "args" prefix, which is a GNU extension, and was not used anyway. Let's just define all of that in a single place. Signed-off-by: Konstantin Kharlamov <Hi-Angel(a)yandex.ru> --- include/wine/debug.h | 42 ++++++++++++++---------------------------- 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/include/wine/debug.h b/include/wine/debug.h index 069e754dfe..e9ec81c623 100644 --- a/include/wine/debug.h +++ b/include/wine/debug.h @@ -69,10 +69,20 @@ struct __wine_debug_channel #ifndef WINE_NO_DEBUG_MSGS # define __WINE_GET_DEBUGGING_WARN(dbch) ((dbch)->flags & (1 << __WINE_DBCL_WARN)) +# define WINE_WARN __WINE_DPRINTF(_WARN,__wine_dbch___default) +# define WINE_WARN_(ch) __WINE_DPRINTF(_WARN,&__wine_dbch_##ch) + +# define WINE_FIXME __WINE_DPRINTF(_FIXME,__wine_dbch___default) +# define WINE_FIXME_(ch) __WINE_DPRINTF(_FIXME,&__wine_dbch_##ch) # define __WINE_GET_DEBUGGING_FIXME(dbch) ((dbch)->flags & (1 << __WINE_DBCL_FIXME)) #else # define __WINE_GET_DEBUGGING_WARN(dbch) 0 +# define WINE_WARN(...) do { } while(0) +# define WINE_WARN_(ch) WINE_WARN + # define __WINE_GET_DEBUGGING_FIXME(dbch) 0 +# define WINE_FIXME(...) do { } while(0) +# define WINE_FIXME_(ch) WINE_FIXME #endif /* define error macro regardless of what is configured */ @@ -96,13 +106,6 @@ struct __wine_debug_channel #define __WINE_PRINTF_ATTR(fmt,args) __attribute__((format (printf,fmt,args))) -#ifdef WINE_NO_DEBUG_MSGS -#define WINE_WARN(args...) do { } while(0) -#define WINE_WARN_(ch) WINE_WARN -#define WINE_FIXME(args...) do { } while(0) -#define WINE_FIXME_(ch) WINE_FIXME -#endif - #elif defined(__SUNPRO_C) #define __WINE_DPRINTF(dbcl,dbch) \ @@ -116,13 +119,6 @@ struct __wine_debug_channel #define __WINE_PRINTF_ATTR(fmt,args) -#ifdef WINE_NO_DEBUG_MSGS -#define WINE_WARN(...) do { } while(0) -#define WINE_WARN_(ch) WINE_WARN -#define WINE_FIXME(...) do { } while(0) -#define WINE_FIXME_(ch) WINE_FIXME -#endif - #else /* !__GNUC__ && !__SUNPRO_C */ #define __WINE_DPRINTF(dbcl,dbch) \ @@ -330,22 +326,12 @@ static inline const char *wine_dbgstr_variant( const VARIANT *v ) #endif /* defined(__oaidl_h__) && defined(V_VT) */ -#define WINE_TRACE_ON(ch) __WINE_IS_DEBUG_ON(_TRACE,&__wine_dbch_##ch) - -#ifndef WINE_WARN -#define WINE_WARN __WINE_DPRINTF(_WARN,__wine_dbch___default) -#define WINE_WARN_(ch) __WINE_DPRINTF(_WARN,&__wine_dbch_##ch) -#endif -#define WINE_WARN_ON(ch) __WINE_IS_DEBUG_ON(_WARN,&__wine_dbch_##ch) - -#ifndef WINE_FIXME -#define WINE_FIXME __WINE_DPRINTF(_FIXME,__wine_dbch___default) -#define WINE_FIXME_(ch) __WINE_DPRINTF(_FIXME,&__wine_dbch_##ch) -#endif -#define WINE_FIXME_ON(ch) __WINE_IS_DEBUG_ON(_FIXME,&__wine_dbch_##ch) - #define WINE_ERR __WINE_DPRINTF(_ERR,__wine_dbch___default) #define WINE_ERR_(ch) __WINE_DPRINTF(_ERR,&__wine_dbch_##ch) + +#define WINE_TRACE_ON(ch) __WINE_IS_DEBUG_ON(_TRACE,&__wine_dbch_##ch) +#define WINE_WARN_ON(ch) __WINE_IS_DEBUG_ON(_WARN,&__wine_dbch_##ch) +#define WINE_FIXME_ON(ch) __WINE_IS_DEBUG_ON(_FIXME,&__wine_dbch_##ch) #define WINE_ERR_ON(ch) __WINE_IS_DEBUG_ON(_ERR,&__wine_dbch_##ch) #define WINE_DECLARE_DEBUG_CHANNEL(ch) \ -- 2.20.1
Below are results of running "the big keybench" benchmark of Xonotic under "perf". They're negligible, still, it might make more difference for slower machines or distinct workloads. branch-misses: vanilla-wine | patched-wine | improvement ------------------------------|------------ 1,757,439,276 | 1,749,075,739 | -8,363,537 1,750,670,918 | 1,746,169,971 | -4,500,947 1,746,488,762 | 1,748,569,739 | 2,080,977 1,753,328,811 | 1,739,515,555 | -13,813,256 Command line example for testing: export WINEPREFIX=~/.wineTESTING; perf stat -B -e cache-misses,branch-misses,faults ~/Projects/wine/test-build/loader/wine ./xonotic-x86.exe -benchmark demos/the-big-keybench Signed-off-by: Konstantin Kharlamov <Hi-Angel(a)yandex.ru> --- include/windef.h | 9 +++++++++ include/wine/debug.h | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/include/windef.h b/include/windef.h index f9a570d9cd..6d1c348d47 100644 --- a/include/windef.h +++ b/include/windef.h @@ -35,6 +35,15 @@ extern "C" { #endif +/* Branch hints for performance critical code */ +#ifdef __GNUC__ +#define __likely(expr) (__builtin_expect(expr, 1)) +#define __unlikely(expr) (__builtin_expect(expr, 0)) +#else +#define __likely(expr) (expr) +#define __unlikely(expr) (expr) +#endif + /* Calling conventions definitions */ #if (defined(__x86_64__) || defined(__powerpc64__) || defined(__aarch64__)) && !defined(_WIN64) diff --git a/include/wine/debug.h b/include/wine/debug.h index e9ec81c623..e13376d6db 100644 --- a/include/wine/debug.h +++ b/include/wine/debug.h @@ -58,7 +58,7 @@ struct __wine_debug_channel }; #ifndef WINE_NO_TRACE_MSGS -# define __WINE_GET_DEBUGGING_TRACE(dbch) ((dbch)->flags & (1 << __WINE_DBCL_TRACE)) +# define __WINE_GET_DEBUGGING_TRACE(dbch) __unlikely((dbch)->flags & (1 << __WINE_DBCL_TRACE)) # define WINE_TRACE __WINE_DPRINTF(_TRACE,__wine_dbch___default) # define WINE_TRACE_(ch) __WINE_DPRINTF(_TRACE,&__wine_dbch_##ch) #else -- 2.20.1
January 26, 2019 4:49 PM, "Konstantin Kharlamov" <Hi-Angel(a)yandex.ru> wrote:
diff --git a/include/wine/debug.h b/include/wine/debug.h index 07ac88d264..069e754dfe 100644 --- a/include/wine/debug.h +++ b/include/wine/debug.h @@ -59,8 +59,12 @@ struct __wine_debug_channel
#ifndef WINE_NO_TRACE_MSGS # define __WINE_GET_DEBUGGING_TRACE(dbch) ((dbch)->flags & (1 << __WINE_DBCL_TRACE)) +# define WINE_TRACE __WINE_DPRINTF(_TRACE,__wine_dbch___default) +# define WINE_TRACE_(ch) __WINE_DPRINTF(_TRACE,&__wine_dbch_##ch) #else # define __WINE_GET_DEBUGGING_TRACE(dbch) 0 +# define WINE_TRACE(args...) do { } while(0)
Missed one. Chip
The complex "#ifdef WINE_NO_TRACE_MSGS && __compiler__" ladder below reduces to "ignore WINE_NO_TRACE_MSGS when !__GNUC__ && !__SUNPRO_C", which is probably a bug. The other nuance is the removed "args" prefix, which is a GNU extension, and was not used anyway. Let's just define all of that in a single place. v2: missed one "args..." in definition (Chip Davis) Signed-off-by: Konstantin Kharlamov <Hi-Angel(a)yandex.ru> --- include/wine/debug.h | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/include/wine/debug.h b/include/wine/debug.h index 07ac88d264..86c7d2fac7 100644 --- a/include/wine/debug.h +++ b/include/wine/debug.h @@ -59,8 +59,12 @@ struct __wine_debug_channel #ifndef WINE_NO_TRACE_MSGS # define __WINE_GET_DEBUGGING_TRACE(dbch) ((dbch)->flags & (1 << __WINE_DBCL_TRACE)) +# define WINE_TRACE __WINE_DPRINTF(_TRACE,__wine_dbch___default) +# define WINE_TRACE_(ch) __WINE_DPRINTF(_TRACE,&__wine_dbch_##ch) #else # define __WINE_GET_DEBUGGING_TRACE(dbch) 0 +# define WINE_TRACE(...) do { } while(0) +# define WINE_TRACE_(ch) WINE_TRACE #endif #ifndef WINE_NO_DEBUG_MSGS @@ -92,12 +96,6 @@ struct __wine_debug_channel #define __WINE_PRINTF_ATTR(fmt,args) __attribute__((format (printf,fmt,args))) - -#ifdef WINE_NO_TRACE_MSGS -#define WINE_TRACE(args...) do { } while(0) -#define WINE_TRACE_(ch) WINE_TRACE -#endif - #ifdef WINE_NO_DEBUG_MSGS #define WINE_WARN(args...) do { } while(0) #define WINE_WARN_(ch) WINE_WARN @@ -118,11 +116,6 @@ struct __wine_debug_channel #define __WINE_PRINTF_ATTR(fmt,args) -#ifdef WINE_NO_TRACE_MSGS -#define WINE_TRACE(...) do { } while(0) -#define WINE_TRACE_(ch) WINE_TRACE -#endif - #ifdef WINE_NO_DEBUG_MSGS #define WINE_WARN(...) do { } while(0) #define WINE_WARN_(ch) WINE_WARN @@ -337,10 +330,6 @@ static inline const char *wine_dbgstr_variant( const VARIANT *v ) #endif /* defined(__oaidl_h__) && defined(V_VT) */ -#ifndef WINE_TRACE -#define WINE_TRACE __WINE_DPRINTF(_TRACE,__wine_dbch___default) -#define WINE_TRACE_(ch) __WINE_DPRINTF(_TRACE,&__wine_dbch_##ch) -#endif #define WINE_TRACE_ON(ch) __WINE_IS_DEBUG_ON(_TRACE,&__wine_dbch_##ch) #ifndef WINE_WARN -- 2.20.1
Konstantin Kharlamov <Hi-Angel(a)yandex.ru> writes:
The complex "#ifdef WINE_NO_TRACE_MSGS && __compiler__" ladder below reduces to "ignore WINE_NO_TRACE_MSGS when !__GNUC__ && !__SUNPRO_C", which is probably a bug.
No, it's on purpose, because some other compiler may not support varargs macros. -- Alexandre Julliard julliard(a)winehq.org
On 28.01.2019 12:24, Alexandre Julliard wrote:
Konstantin Kharlamov <Hi-Angel(a)yandex.ru> writes:
The complex "#ifdef WINE_NO_TRACE_MSGS && __compiler__" ladder below reduces to "ignore WINE_NO_TRACE_MSGS when !__GNUC__ && !__SUNPRO_C", which is probably a bug.
No, it's on purpose, because some other compiler may not support varargs macros.
But then the workaround would result in incorrectly configured build. Is the complexity of the original code really worth the not even correct support of an obscure usecase? That said, looking at definition, I could probably change it to a # define WINE_TRACE(arg1,arg2) do { } while(0) because the other WINE_TRACE definition accepts 2 args. I'll check if it compiles when I get back home. --------- While on it, could you please advice on the naming in the 3-rd patch? Initially I've used "unlikely" because that's how it's used in Mesa. Later then I saw in wine lots of lower-case macros with __ prefix, so I changed it to "__unlikely". But then there are generic macros like "TRACE", and I think of renaming it to "UNLIKELY", which is 2 letters shorter, and is upper-case like yet another part of macros. My personal preference would go to lower-case function-like "unlikely" though.
Konstantin Kharlamov <hi-angel(a)yandex.ru> writes:
On 28.01.2019 12:24, Alexandre Julliard wrote:
Konstantin Kharlamov <Hi-Angel(a)yandex.ru> writes:
The complex "#ifdef WINE_NO_TRACE_MSGS && __compiler__" ladder below reduces to "ignore WINE_NO_TRACE_MSGS when !__GNUC__ && !__SUNPRO_C", which is probably a bug.
No, it's on purpose, because some other compiler may not support varargs macros.
But then the workaround would result in incorrectly configured build. Is the complexity of the original code really worth the not even correct support of an obscure usecase?
WINE_NO_TRACE_MSGS is simply an optimization, there's not much harm if it doesn't work on some obscure compiler. It's better than breaking the build.
While on it, could you please advice on the naming in the 3-rd patch? Initially I've used "unlikely" because that's how it's used in Mesa. Later then I saw in wine lots of lower-case macros with __ prefix, so I changed it to "__unlikely". But then there are generic macros like "TRACE", and I think of renaming it to "UNLIKELY", which is 2 letters shorter, and is upper-case like yet another part of macros.
Considering that the benchmarks don't show an improvement, I don't think it's worth the trouble. Also, adding macros that don't exist on Windows to standard headers is strongly discouraged. -- Alexandre Julliard julliard(a)winehq.org
On 28.01.2019 19:04, Alexandre Julliard wrote:
Konstantin Kharlamov <hi-angel(a)yandex.ru> writes:
On 28.01.2019 12:24, Alexandre Julliard wrote:
Konstantin Kharlamov <Hi-Angel(a)yandex.ru> writes:
The complex "#ifdef WINE_NO_TRACE_MSGS && __compiler__" ladder below reduces to "ignore WINE_NO_TRACE_MSGS when !__GNUC__ && !__SUNPRO_C", which is probably a bug.
No, it's on purpose, because some other compiler may not support varargs macros.
But then the workaround would result in incorrectly configured build. Is the complexity of the original code really worth the not even correct support of an obscure usecase?
WINE_NO_TRACE_MSGS is simply an optimization, there's not much harm if it doesn't work on some obscure compiler. It's better than breaking the build.
While on it, could you please advice on the naming in the 3-rd patch? Initially I've used "unlikely" because that's how it's used in Mesa. Later then I saw in wine lots of lower-case macros with __ prefix, so I changed it to "__unlikely". But then there are generic macros like "TRACE", and I think of renaming it to "UNLIKELY", which is 2 letters shorter, and is upper-case like yet another part of macros.
Considering that the benchmarks don't show an improvement, I don't think it's worth the trouble.
Wait, how so if it does? Besides, even without the benchmark it's obvious that the TRACE appear in so many places that there inevitably would be branch-misses. In addition, this macro can be used for further optimization of performance critical parts of wine code. And what you mean by "the trouble" — it's just a few lines of code that doesn't add any maintainance burden.
Also, adding macros that don't exist on Windows to standard headers is strongly discouraged.
Ah, I see, you mean that `windef.h` is a standard header from WinAPI. Okay, where then should I put it? windef.h just turned out to be one of 2 headers included by debug.h (the other being stdarg.h). I could put it directly into debug.h though.
Konstantin Kharlamov <hi-angel(a)yandex.ru> writes:
On 28.01.2019 19:04, Alexandre Julliard wrote:
Konstantin Kharlamov <hi-angel(a)yandex.ru> writes:
While on it, could you please advice on the naming in the 3-rd patch? Initially I've used "unlikely" because that's how it's used in Mesa. Later then I saw in wine lots of lower-case macros with __ prefix, so I changed it to "__unlikely". But then there are generic macros like "TRACE", and I think of renaming it to "UNLIKELY", which is 2 letters shorter, and is upper-case like yet another part of macros.
Considering that the benchmarks don't show an improvement, I don't think it's worth the trouble.
Wait, how so if it does? Besides, even without the benchmark it's obvious that the TRACE appear in so many places that there inevitably would be branch-misses.
In addition, this macro can be used for further optimization of performance critical parts of wine code.
And what you mean by "the trouble" — it's just a few lines of code that doesn't add any maintainance burden.
That specific change is clearly a tiny burden, but it's the accumulation of such micro-optimizations that ends up being a large burden. That's why we require evidence that it actually helps. By trying to measure it, you had the right approach, but you need to accept the measurement results even if they are not what you expected. If you can't demonstrate a clear improvement, then maybe it's not as obviously a gain as it may seem at first glance. If you want to optimize Wine, I'd suggest picking an app that runs slower than it should, and figuring out where the bottlenecks are. These are the places that are worth optimizing. -- Alexandre Julliard julliard(a)winehq.org
On 28.01.2019 22:10, Alexandre Julliard wrote:
Konstantin Kharlamov <hi-angel(a)yandex.ru> writes:
On 28.01.2019 19:04, Alexandre Julliard wrote:
Konstantin Kharlamov <hi-angel(a)yandex.ru> writes:
While on it, could you please advice on the naming in the 3-rd patch? Initially I've used "unlikely" because that's how it's used in Mesa. Later then I saw in wine lots of lower-case macros with __ prefix, so I changed it to "__unlikely". But then there are generic macros like "TRACE", and I think of renaming it to "UNLIKELY", which is 2 letters shorter, and is upper-case like yet another part of macros.
Considering that the benchmarks don't show an improvement, I don't think it's worth the trouble.
Wait, how so if it does? Besides, even without the benchmark it's obvious that the TRACE appear in so many places that there inevitably would be branch-misses.
In addition, this macro can be used for further optimization of performance critical parts of wine code.
And what you mean by "the trouble" — it's just a few lines of code that doesn't add any maintainance burden.
That specific change is clearly a tiny burden, but it's the accumulation of such micro-optimizations that ends up being a large burden. That's why we require evidence that it actually helps.
By trying to measure it, you had the right approach, but you need to accept the measurement results even if they are not what you expected. If you can't demonstrate a clear improvement, then maybe it's not as obviously a gain as it may seem at first glance.
If you want to optimize Wine, I'd suggest picking an app that runs slower than it should, and figuring out where the bottlenecks are. These are the places that are worth optimizing.
5 millions branch-misses less on average is a clear improvement. Besides you need to consider that given TRACE is used a lot in the code, there clearly can be workloads where TRACE is executed more often, and hence results in slower execution. I also don't see why would it affect a burden increase in the future. If you're referring to using "unlikely" in more places in the code — I can assure you, it doesn't make it any less readable, you can see examples here https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/gallium/drivers/r60...
On 28.01.2019 19:04, Alexandre Julliard wrote:
Konstantin Kharlamov <hi-angel(a)yandex.ru> writes:
On 28.01.2019 12:24, Alexandre Julliard wrote:
Konstantin Kharlamov <Hi-Angel(a)yandex.ru> writes:
The complex "#ifdef WINE_NO_TRACE_MSGS && __compiler__" ladder below reduces to "ignore WINE_NO_TRACE_MSGS when !__GNUC__ && !__SUNPRO_C", which is probably a bug.
No, it's on purpose, because some other compiler may not support varargs macros.
But then the workaround would result in incorrectly configured build. Is the complexity of the original code really worth the not even correct support of an obscure usecase?
WINE_NO_TRACE_MSGS is simply an optimization, there's not much harm if it doesn't work on some obscure compiler. It's better than breaking the build.
Okay, thanks for your comments. I just checked whether I can just pass static number of arguments, and I can't — the other definition of TRACE unrolls to a macros with variadic args, but only for supported compilers. I could probably define multiple macros with predefined number of args, but I'm not sure if anybody wants this refactoring, so I'd just drop the patches. I still would like to hear your opinion on my reply in the other mail, since as a Wine user I still would like to optimize it, and hence to get the 3rd patch into the project.
Thanks! I resent the v2 of the patch On 27.01.2019 06:35, Chip Davis wrote:
January 26, 2019 4:49 PM, "Konstantin Kharlamov" <Hi-Angel(a)yandex.ru> wrote:
diff --git a/include/wine/debug.h b/include/wine/debug.h index 07ac88d264..069e754dfe 100644 --- a/include/wine/debug.h +++ b/include/wine/debug.h @@ -59,8 +59,12 @@ struct __wine_debug_channel
#ifndef WINE_NO_TRACE_MSGS # define __WINE_GET_DEBUGGING_TRACE(dbch) ((dbch)->flags & (1 << __WINE_DBCL_TRACE)) +# define WINE_TRACE __WINE_DPRINTF(_TRACE,__wine_dbch___default) +# define WINE_TRACE_(ch) __WINE_DPRINTF(_TRACE,&__wine_dbch_##ch) #else # define __WINE_GET_DEBUGGING_TRACE(dbch) 0 +# define WINE_TRACE(args...) do { } while(0)
Missed one.
Chip
participants (3)
-
Alexandre Julliard -
Chip Davis -
Konstantin Kharlamov