Re: [RFC/PATCH v4 2/3] add trace_performance facility to debug performance issues

2014-05-21 Thread Jeff King
On Tue, May 20, 2014 at 09:11:19PM +0200, Karsten Blees wrote:

 Add trace_performance and trace_performance_since macros that print file
 name, line number, time and an optional printf-formatted text to the file
 specified in environment variable GIT_TRACE_PERFORMANCE.
 
 Unless enabled via GIT_TRACE_PERFORMANCE, these macros have no noticeable
 impact on performance, so that test code may be shipped in release builds.
 
 MSVC: variadic macros (__VA_ARGS__) require VC++ 2005 or newer.

I think we still have some Unix compilers that do not do variadic
macros, either. For a while, people were compiling with antique stuff
like SUNWspro and MIPSpro. I don't know if they still do, if they use
gcc on such systems now, or if those systems have finally been
decomissioned.

But either we need to change our stance on variadic macros, or this
feature needs to be able to be compiled conditionally.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH v4 2/3] add trace_performance facility to debug performance issues

2014-05-21 Thread Karsten Blees
Am 21.05.2014 18:58, schrieb Jeff King:
 On Tue, May 20, 2014 at 09:11:19PM +0200, Karsten Blees wrote:
 
 Add trace_performance and trace_performance_since macros that print file
 name, line number, time and an optional printf-formatted text to the file
 specified in environment variable GIT_TRACE_PERFORMANCE.

 Unless enabled via GIT_TRACE_PERFORMANCE, these macros have no noticeable
 impact on performance, so that test code may be shipped in release builds.

 MSVC: variadic macros (__VA_ARGS__) require VC++ 2005 or newer.
 
 I think we still have some Unix compilers that do not do variadic
 macros, either. For a while, people were compiling with antique stuff
 like SUNWspro and MIPSpro. I don't know if they still do, if they use
 gcc on such systems now, or if those systems have finally been
 decomissioned.
 
 But either we need to change our stance on variadic macros, or this
 feature needs to be able to be compiled conditionally.
 
 -Peff
 

Macros are mainly used to supply __FILE__ and __LINE__, so that lazy people 
don't need to think of a unique message for each use of trace_performance_*. 
Without __FILE__, __LINE__ and message, the output would be pretty useless 
(i.e. just the time without any additional info).

If there's platforms that don't support variadic macros, I'd suggest to drop 
the __FILE__ __LINE__ feature completely and make message mandatory (with the 
added benefit that manually provided messages don't change if the code is 
moved, i.e. trace logs would become somewhat comparable across versions).

(adding cc: Dscho as IIRC the __FILE__ __LINE__ idea was originally his).

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC/PATCH v4 2/3] add trace_performance facility to debug performance issues

2014-05-21 Thread Jeff King
On Wed, May 21, 2014 at 08:34:47PM +0200, Karsten Blees wrote:

 Macros are mainly used to supply __FILE__ and __LINE__, so that lazy
 people don't need to think of a unique message for each use of
 trace_performance_*. Without __FILE__, __LINE__ and message, the
 output would be pretty useless (i.e. just the time without any
 additional info).
 
 If there's platforms that don't support variadic macros, I'd suggest
 to drop the __FILE__ __LINE__ feature completely and make message
 mandatory (with the added benefit that manually provided messages
 don't change if the code is moved, i.e. trace logs would become
 somewhat comparable across versions).

I do think __FILE__ and __LINE__ can be useful, and it would not be the
end of the world to simply omit them on platforms that can't do the
variadic macros (there shouldn't be many these days, and I think they
are used to living with reduced functionality in some cases).

But if this were attached to trace_printf, we would always have a
message anyway, no?

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html