Re: [PATCH 2/2] trace: improve performance while category is disabled

2017-11-27 Thread Gennady Kupava 
> Spotted yet another.  This function in a header file, that is
included by many source files, must be made "static inline" (which I
already did as without the fix I couldn't get 'pu' to compile).

Thanks, missed that, seems my compiler inlined all calls and I didn't
notice the problem.


On 27 November 2017 at 03:25, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Just in case others notice style and whitespace issues, I've applied
>> the following to fix them, so there is no need to reroll only to fix
>> these.
>> ...
>> -inline int trace_pass_fl(struct trace_key *key) {
>> +inline int trace_pass_fl(struct trace_key *key)
>> +{
>>   return key->fd || !key->initialized;
>>  }
>
> Spotted yet another.  This function in a header file, that is
> included by many source files, must be made "static inline" (which I
> already did as without the fix I couldn't get 'pu' to compile).
>
>
>

[PATCH 1/2] trace: remove trace key normalization

2017-11-26 Thread gennady . kupava 
From: Gennady Kupava <gkup...@bloomberg.net>

Trace key normalization is not used, not strictly necessary,
complicates the code and would negatively affect compilation speed if
moved to header.

New trace_default_key key or existing separate marco could be used
instead of passing NULL as a key.

Signed-off-by: Gennady Kupava <gkup...@bloomberg.net>
---
 trace.c | 24 
 trace.h |  4 +++-
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/trace.c b/trace.c
index cb1293ed3..d47ea28e8 100644
--- a/trace.c
+++ b/trace.c
@@ -24,26 +24,13 @@
 #include "cache.h"
 #include "quote.h"
 
-/*
- * "Normalize" a key argument by converting NULL to our trace_default,
- * and otherwise passing through the value. All caller-facing functions
- * should normalize their inputs in this way, though most get it
- * for free by calling get_trace_fd() (directly or indirectly).
- */
-static void normalize_trace_key(struct trace_key **key)
-{
-   static struct trace_key trace_default = { "GIT_TRACE" };
-   if (!*key)
-   *key = _default;
-}
+struct trace_key trace_default_key = { "GIT_TRACE", 0, 0, 0 };
 
 /* Get a trace file descriptor from "key" env variable. */
 static int get_trace_fd(struct trace_key *key)
 {
const char *trace;
 
-   normalize_trace_key();
-
/* don't open twice */
if (key->initialized)
return key->fd;
@@ -81,8 +68,6 @@ static int get_trace_fd(struct trace_key *key)
 
 void trace_disable(struct trace_key *key)
 {
-   normalize_trace_key();
-
if (key->need_close)
close(key->fd);
key->fd = 0;
@@ -128,7 +113,6 @@ static int prepare_trace_line(const char *file, int line,
 static void trace_write(struct trace_key *key, const void *buf, unsigned len)
 {
if (write_in_full(get_trace_fd(key), buf, len) < 0) {
-   normalize_trace_key();
warning("unable to write trace for %s: %s",
key->key, strerror(errno));
trace_disable(key);
@@ -167,13 +151,13 @@ static void trace_argv_vprintf_fl(const char *file, int 
line,
 {
struct strbuf buf = STRBUF_INIT;
 
-   if (!prepare_trace_line(file, line, NULL, ))
+   if (!prepare_trace_line(file, line, _default_key, ))
return;
 
strbuf_vaddf(, format, ap);
 
sq_quote_argv(, argv, 0);
-   print_trace_line(NULL, );
+   print_trace_line(_default_key, );
 }
 
 void trace_strbuf_fl(const char *file, int line, struct trace_key *key,
@@ -215,7 +199,7 @@ void trace_printf(const char *format, ...)
 {
va_list ap;
va_start(ap, format);
-   trace_vprintf_fl(NULL, 0, NULL, format, ap);
+   trace_vprintf_fl(NULL, 0, _default_key, format, ap);
va_end(ap);
 }
 
diff --git a/trace.h b/trace.h
index 179b249c5..24b32f8f4 100644
--- a/trace.h
+++ b/trace.h
@@ -11,6 +11,8 @@ struct trace_key {
unsigned int  need_close : 1;
 };
 
+extern struct trace_key trace_default_key;
+
 #define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
 
 extern void trace_repo_setup(const char *prefix);
@@ -78,7 +80,7 @@ extern void trace_performance_since(uint64_t start, const 
char *format, ...);
  */
 
 #define trace_printf(...) \
-   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)
+   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, _default_key, 
__VA_ARGS__)
 
 #define trace_printf_key(key, ...) \
trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__)
-- 
2.14.1

[PATCH 2/2] trace: improve performance while category is disabled

2017-11-26 Thread gennady . kupava 
From: Gennady Kupava <gkup...@bloomberg.net>

- Do the check if the trace key is enabled sooner in call chain.
- Move just enough code from trace.c into trace.h header so all code
  necessary to determine that trace is disabled could be inlined to
  calling functions.

Signed-off-by: Gennady Kupava <gkup...@bloomberg.net>
---
 trace.c |  3 +--
 trace.h | 58 --
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/trace.c b/trace.c
index d47ea28e8..b7530b51a 100644
--- a/trace.c
+++ b/trace.c
@@ -25,6 +25,7 @@
 #include "quote.h"
 
 struct trace_key trace_default_key = { "GIT_TRACE", 0, 0, 0 };
+struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
 
 /* Get a trace file descriptor from "key" env variable. */
 static int get_trace_fd(struct trace_key *key)
@@ -172,8 +173,6 @@ void trace_strbuf_fl(const char *file, int line, struct 
trace_key *key,
print_trace_line(key, );
 }
 
-static struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
-
 static void trace_performance_vprintf_fl(const char *file, int line,
 uint64_t nanos, const char *format,
 va_list ap)
diff --git a/trace.h b/trace.h
index 24b32f8f4..db10f2afe 100644
--- a/trace.h
+++ b/trace.h
@@ -14,6 +14,7 @@ struct trace_key {
 extern struct trace_key trace_default_key;
 
 #define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
+extern struct trace_key trace_perf_key;
 
 extern void trace_repo_setup(const char *prefix);
 extern int trace_want(struct trace_key *key);
@@ -79,24 +80,42 @@ extern void trace_performance_since(uint64_t start, const 
char *format, ...);
  * comma, but this is non-standard.
  */
 
-#define trace_printf(...) \
-   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, _default_key, 
__VA_ARGS__)
-
-#define trace_printf_key(key, ...) \
-   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__)
-
-#define trace_argv_printf(argv, ...) \
-   trace_argv_printf_fl(TRACE_CONTEXT, __LINE__, argv, __VA_ARGS__)
-
-#define trace_strbuf(key, data) \
-   trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data)
-
-#define trace_performance(nanos, ...) \
-   trace_performance_fl(TRACE_CONTEXT, __LINE__, nanos, __VA_ARGS__)
-
-#define trace_performance_since(start, ...) \
-   trace_performance_fl(TRACE_CONTEXT, __LINE__, getnanotime() - (start), \
-__VA_ARGS__)
+#define trace_printf_key(key, ...) \
+   do {\
+   if (trace_pass_fl(key)) \
+   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key,   \
+   __VA_ARGS__);   \
+   } while(0)
+
+#define trace_printf(...) trace_printf_key(_default_key, __VA_ARGS__)
+
+#define trace_argv_printf(argv, ...)   \
+   do {\
+   if (trace_pass_fl(_default_key))  \
+  trace_argv_printf_fl(TRACE_CONTEXT, __LINE__,\
+   argv, __VA_ARGS__); \
+   } while(0)
+
+#define trace_strbuf(key, data)
\
+   do {\
+   if (trace_pass_fl(key)) \
+   trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\
+   } while(0)
+
+#define trace_performance(nanos, ...)  \
+   do {\
+   if (trace_pass_fl(_perf_key)) \
+   trace_performance_fl(TRACE_CONTEXT, __LINE__, nanos,\
+__VA_ARGS__);  \
+   } while(0)
+
+#define trace_performance_since(start, ...)\
+   do {\
+   if (trace_pass_fl(_perf_key)) \
+   trace_performance_fl(TRACE_CONTEXT, __LINE__,   \
+getnanotime() - (start),   \
+__VA_ARGS__);  \
+   } while(0)
 
 /* backend functions, use non-*fl macros instead */
 __attribute__((format (printf, 4, 5)))
@@ -110,6 +129,9 @@ extern void trace_strbuf_fl(const char *file, int line, 
struct trace_key *key,
 __attribute__((format (printf, 4, 5)))
 extern void trace_performance_fl(const char *file, int line,

Re: [PATCH 2/2] Reduce performance cost of the trace if trace category is disabled

2017-11-19 Thread Gennady Kupava 
Right, this trace is actually not used anywhere, so only check was
compilation. Will fix that.

On 19 November 2017 at 08:27, Johannes Sixt  wrote:
> Am 19.11.2017 um 01:42 schrieb gennady.kup...@gmail.com:
>>
>> +#define trace_printf_key(key, ...)
>> \
>> +   do {
>> \
>> +   if (trace_pass_fl(key))
>> \
>> +   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key,
>> \
>> +   __VA_ARGS__);
>> \
>> +   } while(0)
>> +
>> +#define trace_printf(...) trace_printf_key(_default_key,
>> __VA_ARGS__);
>> +
>> +#define trace_argv_printf(argv, ...)
>> \
>> +   do {
>> \
>> +   if (trace_pass_fl(_default_key))
>> \
>> +  trace_argv_printf_fl(TRACE_CONTEXT, __LINE__,
>> \
>> +   argv, __VA_ARGS__);
>> \
>> +   } while(0)
>> +
>> +#define trace_strbuf(key, data)
>> \
>> +   do {
>> \
>> +   if (trace_pass_fl(key))
>> \
>> +   trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key,
>> data);\
>> +   } while(0)
>> +
>> +#define trace_performance(nanos, ...)
>> \
>> +   do {
>> \
>> +   if (trace_pass_fl(key))
>> \
>
>
> The token "key" here looks suspicious. Did you mean _perf_key?
>
>> +   trace_performance_fl(TRACE_CONTEXT, __LINE__,
>> nanos,\
>> +__VA_ARGS__);
>> \
>> +   } while(0)
>> +
>> +#define trace_performance_since(start, ...)
>> \
>> +   do {
>> \
>> +   if (trace_pass_fl(_perf_key))
>> \
>> +   trace_performance_fl(TRACE_CONTEXT, __LINE__,
>> \
>> +getnanotime() - (start),
>> \
>> +__VA_ARGS__);
>> \
>> +   } while(0)
>> /* backend functions, use non-*fl macros instead */
>>   __attribute__((format (printf, 4, 5)))
>
>
> -- Hannes

Re: [PATCH 1/2] Simplify tracing code by removing trace key normalization concept

2017-11-19 Thread Gennady Kupava 
> The usual style comment on the subject applies here.

Oh sure, 50 characters. 'Remove trace key normalization concept' would
be better?

> I cannot quite tell what it is trying to achive to make it a
bulleted list.  It's not like four things at the same conceptual
level is enumerated; instead it just has four sentences that talk
about random things.

With these four sentences I am describing the reasons why we need this
patch. In my previous iteration I had similar  description outside of
git comment and was told to move it into git description. So it is in
comments. Hot sure if I understand what is the problem here. I can
remove bulleted list and make it four sentences. I can remove it
altogether. I can add some text so it will be easier to understand
this without context of previous patch. What would be the best?

I reread relevant parts of
https://github.com/git/git/blob/master/Documentation/SubmittingPatches
and it seems description of the reasons and alternative discarded
solutions fits into description. So could please help me a bit to
understand what is wrong.

> More importantly, I am not sure I understand what these sentences
are trying to say.  "Should be moved to header"---so?  Does that
move something from the source to the header?  It seems to me that
the patch removes a helper function from trace.c but does not add
anything to the header.

I was told to split the patch and do removal for the normalization as
separate commit. As it is separate commit, it needs to do separate
description with the own reasons why etc. Yes it doesn't but it is
needed for the second patch in the series.

So comments explaining that, while implementing trace optimization, I
saw two options:
1. Move normalization function from .c file to .h file
2. Remove it

And why I choose removal - not used, would complicate header without
any benefit, and actually I didn't mention minor benefit of
compilation speed. This trace.h included and used in lots of places so
it will take compiler some time to actually eliminate the code.

> Puzzled.

Does it make more sense now?

Gennady


On 19 November 2017 at 02:19, Junio C Hamano <gits...@pobox.com> wrote:
> gennady.kup...@gmail.com writes:
>
>> Subject: Re: [PATCH 1/2] Simplify tracing code by removing trace key 
>> normalization concept
>
> The usual style comment on the subject applies here.
>
>> From: Gennady Kupava <gkup...@bloomberg.net>
>>
>> - to implement efficient traces with normalization, normalization
>>   implementation should be moved to header. as it seems better to not
>>   overload header file with this normalization logic, suggestion is
>>   just to remove it
>> - different macro exist specifically to handle traces with default key
>> - there is no use of normalization in current code
>> - it could be reintroduced if necessary
>
> I cannot quite tell what it is trying to achive to make it a
> bulleted list.  It's not like four things at the same conceptual
> level is enumerated; instead it just has four sentences that talk
> about random things.
>
> More importantly, I am not sure I understand what these sentences
> are trying to say.  "Should be moved to header"---so?  Does that
> move something from the source to the header?  It seems to me that
> the patch removes a helper function from trace.c but does not add
> anything to the header.
>
> Or am I wasting everybody's time by commenting on a stale comment
> that used to describe an ancient iteration of this code?
>
> Puzzled.

[PATCH 2/2] Reduce performance cost of the trace if trace category is disabled

2017-11-18 Thread gennady . kupava 
From: Gennady Kupava <gkup...@bloomberg.net>

- Do the check if the trace key is enabled sooner in call chain.
- Move just enough code from trace.c into trace.h header so all code
  necessary to determine that trace is disabled could be inlined to
  calling functions.

Signed-off-by: Gennady Kupava <gkup...@bloomberg.net>
---
 trace.c |  3 +--
 trace.h | 58 --
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/trace.c b/trace.c
index d47ea28e8..b7530b51a 100644
--- a/trace.c
+++ b/trace.c
@@ -25,6 +25,7 @@
 #include "quote.h"
 
 struct trace_key trace_default_key = { "GIT_TRACE", 0, 0, 0 };
+struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
 
 /* Get a trace file descriptor from "key" env variable. */
 static int get_trace_fd(struct trace_key *key)
@@ -172,8 +173,6 @@ void trace_strbuf_fl(const char *file, int line, struct 
trace_key *key,
print_trace_line(key, );
 }
 
-static struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
-
 static void trace_performance_vprintf_fl(const char *file, int line,
 uint64_t nanos, const char *format,
 va_list ap)
diff --git a/trace.h b/trace.h
index 24b32f8f4..cd9e280ba 100644
--- a/trace.h
+++ b/trace.h
@@ -14,6 +14,7 @@ struct trace_key {
 extern struct trace_key trace_default_key;
 
 #define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
+extern struct trace_key trace_perf_key;
 
 extern void trace_repo_setup(const char *prefix);
 extern int trace_want(struct trace_key *key);
@@ -79,24 +80,42 @@ extern void trace_performance_since(uint64_t start, const 
char *format, ...);
  * comma, but this is non-standard.
  */
 
-#define trace_printf(...) \
-   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, _default_key, 
__VA_ARGS__)
-
-#define trace_printf_key(key, ...) \
-   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__)
-
-#define trace_argv_printf(argv, ...) \
-   trace_argv_printf_fl(TRACE_CONTEXT, __LINE__, argv, __VA_ARGS__)
-
-#define trace_strbuf(key, data) \
-   trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data)
-
-#define trace_performance(nanos, ...) \
-   trace_performance_fl(TRACE_CONTEXT, __LINE__, nanos, __VA_ARGS__)
-
-#define trace_performance_since(start, ...) \
-   trace_performance_fl(TRACE_CONTEXT, __LINE__, getnanotime() - (start), \
-__VA_ARGS__)
+#define trace_printf_key(key, ...) \
+   do {\
+   if (trace_pass_fl(key)) \
+   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key,   \
+   __VA_ARGS__);   \
+   } while(0)
+
+#define trace_printf(...) trace_printf_key(_default_key, __VA_ARGS__);
+
+#define trace_argv_printf(argv, ...)   \
+   do {\
+   if (trace_pass_fl(_default_key))  \
+  trace_argv_printf_fl(TRACE_CONTEXT, __LINE__,\
+   argv, __VA_ARGS__); \
+   } while(0)
+
+#define trace_strbuf(key, data)
\
+   do {\
+   if (trace_pass_fl(key)) \
+   trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\
+   } while(0)
+
+#define trace_performance(nanos, ...)  \
+   do {\
+   if (trace_pass_fl(key)) \
+   trace_performance_fl(TRACE_CONTEXT, __LINE__, nanos,\
+__VA_ARGS__);  \
+   } while(0)
+
+#define trace_performance_since(start, ...)\
+   do {\
+   if (trace_pass_fl(_perf_key)) \
+   trace_performance_fl(TRACE_CONTEXT, __LINE__,   \
+getnanotime() - (start),   \
+__VA_ARGS__);  \
+   } while(0)
 
 /* backend functions, use non-*fl macros instead */
 __attribute__((format (printf, 4, 5)))
@@ -110,6 +129,9 @@ extern void trace_strbuf_fl(const char *file, int line, 
struct trace_key *key,
 __attribute__((format (printf, 4, 5)))
 extern void trace_performance_fl(const char *file, int line,

[PATCH 1/2] Simplify tracing code by removing trace key normalization concept

2017-11-18 Thread gennady . kupava 
From: Gennady Kupava <gkup...@bloomberg.net>

- to implement efficient traces with normalization, normalization
  implementation should be moved to header. as it seems better to not
  overload header file with this normalization logic, suggestion is
  just to remove it
- different macro exist specifically to handle traces with default key
- there is no use of normalization in current code
- it could be reintroduced if necessary

Signed-off-by: Gennady Kupava <gkup...@bloomberg.net>
---
 trace.c | 24 
 trace.h |  4 +++-
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/trace.c b/trace.c
index cb1293ed3..d47ea28e8 100644
--- a/trace.c
+++ b/trace.c
@@ -24,26 +24,13 @@
 #include "cache.h"
 #include "quote.h"
 
-/*
- * "Normalize" a key argument by converting NULL to our trace_default,
- * and otherwise passing through the value. All caller-facing functions
- * should normalize their inputs in this way, though most get it
- * for free by calling get_trace_fd() (directly or indirectly).
- */
-static void normalize_trace_key(struct trace_key **key)
-{
-   static struct trace_key trace_default = { "GIT_TRACE" };
-   if (!*key)
-   *key = _default;
-}
+struct trace_key trace_default_key = { "GIT_TRACE", 0, 0, 0 };
 
 /* Get a trace file descriptor from "key" env variable. */
 static int get_trace_fd(struct trace_key *key)
 {
const char *trace;
 
-   normalize_trace_key();
-
/* don't open twice */
if (key->initialized)
return key->fd;
@@ -81,8 +68,6 @@ static int get_trace_fd(struct trace_key *key)
 
 void trace_disable(struct trace_key *key)
 {
-   normalize_trace_key();
-
if (key->need_close)
close(key->fd);
key->fd = 0;
@@ -128,7 +113,6 @@ static int prepare_trace_line(const char *file, int line,
 static void trace_write(struct trace_key *key, const void *buf, unsigned len)
 {
if (write_in_full(get_trace_fd(key), buf, len) < 0) {
-   normalize_trace_key();
warning("unable to write trace for %s: %s",
key->key, strerror(errno));
trace_disable(key);
@@ -167,13 +151,13 @@ static void trace_argv_vprintf_fl(const char *file, int 
line,
 {
struct strbuf buf = STRBUF_INIT;
 
-   if (!prepare_trace_line(file, line, NULL, ))
+   if (!prepare_trace_line(file, line, _default_key, ))
return;
 
strbuf_vaddf(, format, ap);
 
sq_quote_argv(, argv, 0);
-   print_trace_line(NULL, );
+   print_trace_line(_default_key, );
 }
 
 void trace_strbuf_fl(const char *file, int line, struct trace_key *key,
@@ -215,7 +199,7 @@ void trace_printf(const char *format, ...)
 {
va_list ap;
va_start(ap, format);
-   trace_vprintf_fl(NULL, 0, NULL, format, ap);
+   trace_vprintf_fl(NULL, 0, _default_key, format, ap);
va_end(ap);
 }
 
diff --git a/trace.h b/trace.h
index 179b249c5..24b32f8f4 100644
--- a/trace.h
+++ b/trace.h
@@ -11,6 +11,8 @@ struct trace_key {
unsigned int  need_close : 1;
 };
 
+extern struct trace_key trace_default_key;
+
 #define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
 
 extern void trace_repo_setup(const char *prefix);
@@ -78,7 +80,7 @@ extern void trace_performance_since(uint64_t start, const 
char *format, ...);
  */
 
 #define trace_printf(...) \
-   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)
+   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, _default_key, 
__VA_ARGS__)
 
 #define trace_printf_key(key, ...) \
trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__)
-- 
2.14.1

Re: [PATCH] Reduce performance penalty for turned off traces

2017-11-12 Thread Gennady Kupava 
Hi Jeff, thanks for such detailed review and additional testing.

Below are just some discussion related review comments,
Please expect patch itself on some evening during coming week.

On 12 November 2017 at 14:17, Jeff King <p...@peff.net> wrote:
>
> On Sat, Nov 11, 2017 at 07:28:58PM +, gennady.kup...@gmail.com wrote:
>
> > From: Gennady Kupava <gkup...@bloomberg.net>
> >
> > Signed-off-by: Gennady Kupava <gkup...@bloomberg.net>
>
> Thanks, and welcome to the list.
>
>
> > ---
> > One of the tasks siggested in scope of 'Git sprint weekend'.
> > Couple of chioces made here:
>
> These kinds of choices/caveats (along with the motivation for the patch)
> should go into the commit message so people running "git log" later can
> see them.

Will split into two patches.

>
> >  1. Use constant instead of NULL to indicate default trace level, this just
> > makes everything simpler.
>
> I think this makes sense, as we were translating NULL into this default
> struct behind the scenes already. As we discussed off-list, this does
> mean that you can no longer do:
>
>   trace_printf_key(NULL, "foo");
>
> to send to the default key, and instead must do:
>
>   trace_printf("foo");
>
> The second one has always been the preferred way of spelling it, but the
> first one happened to work. So the only fallout would be if somebody is
> using the non-preferred method, they'd now segfault. There aren't any
> such calls in the current code base, though, and I find it rather
> unlikely that there would be new instances in topics other people are
> working on.
>
> I think it may be worth splitting that out into a separate preparatory
> patch to make it more clear what's going on (especially if our
> assumption turns out wrong and somebody does end up tracing a problem
> back to it).


Logic which lead me to removing NULL:
_If_ we want to do some kind of trivial check before calling functions
from other
translation units (which is always real function call), we have to
have some kind
of status immediately available in .h file.
I saw two options here:
 - move whole 'normalization' procedure to .h
 - remove 'normalization' procedure

Reasons removal option was chosen:
 - I found no code which actually uses NULL as a parameter option.
 - Given the nature of the key parameter ( struct key* )
I was just really unable to find any reasonable use
case there caller really want to pass NULL, so it seemed to me that
best solution
is to remove it. I still can imagine something like dynamic trace
category coming from
some kind of configuration file. In such special case there caller
might need it,
he might just do check in calling code once or just simply add it.
 - Squash big more performance as we do not need to do this
normalization on every call.

I will elaborate on 'moving whole implementation later'

>
> >  2. Move just enough from implementation to header, to be able to do check
> > before calling actual functions.
>
> Makes sense.
>
> >  3. Most controversail: do not support optimization for older
> > compilers not supporting variadic templates in macroses. Problem
> > is that theoretically someone may write some complicated trace
> > while would work in 'modern' compiler and be too slow in more
> > 'classic' compilers, however expectation is that risk of that is
> > quite low and 'classic' compilers will go away eventually.
>
> I think this is probably acceptable. I know we've discussed elsewhere
> recently how common such compilers actually are, and whether we could
> give up on them altogether.
>
> If we wanted to, we could support that case by using inline functions in
> the header (though it does make me wonder if compilers that old also do
> not actually end up inlining).
>
> I did manually disable HAVE_VARIADIC_MACROS and confirmed that the
> result builds and passes the test suite (though I suspect that GIT_TRACE
> is not well exercised by the suite).
>

Good.

>
> > diff --git a/trace.c b/trace.c
> > index 7508aea02..180ee3b00 100644
> > --- a/trace.c
> > +++ b/trace.c
> > @@ -25,26 +25,14 @@
> >  #include "cache.h"
> >  #include "quote.h"
> >
> > -/*
> > - * "Normalize" a key argument by converting NULL to our trace_default,
> > - * and otherwise passing through the value. All caller-facing functions
> > - * should normalize their inputs in this way, though most get it
> > - * for free by calling get_trace_fd() (directly or indirectly).
> > - */
> > -static void normalize_trace_key(struct trace_key **key)
> > -{
> > - static struct trace_key trace_default = { "GIT_T

[PATCH] Reduce performance penalty for turned off traces

2017-11-11 Thread gennady . kupava 
From: Gennady Kupava <gkup...@bloomberg.net>

Signed-off-by: Gennady Kupava <gkup...@bloomberg.net>
---
One of the tasks siggested in scope of 'Git sprint weekend'.
Couple of chioces made here:
 1. Use constant instead of NULL to indicate default trace level, this just
makes everything simpler.
 2. Move just enough from implementation to header, to be able to do check
before calling actual functions.
 3. Most controversail: do not support optimization for older compilers not
supporting variadic templates in macroses. Problem is that theoretically
someone may write some complicated trace while would work in 'modern' compiler 
and be too slow in more 'classic' compilers, however expectation is that risk 
of that is quite low and 'classic' compilers will go away eventually.

Passes test suite locally on Linux/amd64.

 trace.c | 29 ++--
 trace.h | 68 +++--
 2 files changed, 55 insertions(+), 42 deletions(-)

diff --git a/trace.c b/trace.c
index 7508aea02..180ee3b00 100644
--- a/trace.c
+++ b/trace.c
@@ -25,26 +25,14 @@
 #include "cache.h"
 #include "quote.h"
 
-/*
- * "Normalize" a key argument by converting NULL to our trace_default,
- * and otherwise passing through the value. All caller-facing functions
- * should normalize their inputs in this way, though most get it
- * for free by calling get_trace_fd() (directly or indirectly).
- */
-static void normalize_trace_key(struct trace_key **key)
-{
-   static struct trace_key trace_default = { "GIT_TRACE" };
-   if (!*key)
-   *key = _default;
-}
+struct trace_key trace_default_key = { TRACE_KEY_PREFIX, 0, 0, 0 };
+struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
 
 /* Get a trace file descriptor from "key" env variable. */
 static int get_trace_fd(struct trace_key *key)
 {
const char *trace;
 
-   normalize_trace_key();
-
/* don't open twice */
if (key->initialized)
return key->fd;
@@ -82,8 +70,6 @@ static int get_trace_fd(struct trace_key *key)
 
 void trace_disable(struct trace_key *key)
 {
-   normalize_trace_key();
-
if (key->need_close)
close(key->fd);
key->fd = 0;
@@ -129,7 +115,6 @@ static int prepare_trace_line(const char *file, int line,
 static void trace_write(struct trace_key *key, const void *buf, unsigned len)
 {
if (write_in_full(get_trace_fd(key), buf, len) < 0) {
-   normalize_trace_key();
warning("unable to write trace for %s: %s",
key->key, strerror(errno));
trace_disable(key);
@@ -168,13 +153,13 @@ static void trace_argv_vprintf_fl(const char *file, int 
line,
 {
struct strbuf buf = STRBUF_INIT;
 
-   if (!prepare_trace_line(file, line, NULL, ))
+   if (!prepare_trace_line(file, line, _default_key, ))
return;
 
strbuf_vaddf(, format, ap);
 
sq_quote_argv(, argv, 0);
-   print_trace_line(NULL, );
+   print_trace_line(_default_key, );
 }
 
 void trace_strbuf_fl(const char *file, int line, struct trace_key *key,
@@ -189,8 +174,6 @@ void trace_strbuf_fl(const char *file, int line, struct 
trace_key *key,
print_trace_line(key, );
 }
 
-static struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
-
 static void trace_performance_vprintf_fl(const char *file, int line,
 uint64_t nanos, const char *format,
 va_list ap)
@@ -216,7 +199,7 @@ void trace_printf(const char *format, ...)
 {
va_list ap;
va_start(ap, format);
-   trace_vprintf_fl(NULL, 0, NULL, format, ap);
+   trace_vprintf_fl(NULL, 0, _default_key, format, ap);
va_end(ap);
 }
 
@@ -341,7 +324,7 @@ void trace_repo_setup(const char *prefix)
 
 int trace_want(struct trace_key *key)
 {
-   return !!get_trace_fd(key);
+   return !!get_trace_fd(key);
 }
 
 #if defined(HAVE_CLOCK_GETTIME) && defined(HAVE_CLOCK_MONOTONIC)
diff --git a/trace.h b/trace.h
index 179b249c5..64326573a 100644
--- a/trace.h
+++ b/trace.h
@@ -4,6 +4,8 @@
 #include "git-compat-util.h"
 #include "strbuf.h"
 
+#define TRACE_KEY_PREFIX "GIT_TRACE"
+
 struct trace_key {
const char * const key;
int fd;
@@ -11,7 +13,10 @@ struct trace_key {
unsigned int  need_close : 1;
 };
 
-#define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 }
+#define TRACE_KEY_INIT(name) { TRACE_KEY_PREFIX "_" #name, 0, 0, 0 }
+
+extern struct trace_key trace_default_key;
+extern struct trace_key trace_perf_key;
 
 extern void trace_repo_setup(const char *prefix);
 extern int trace_want(struct trace_key *key);
@@ -77,24 +82,49 @@ extern void trace_performance_since(uint64_t start, const 
char *format, ...);
  *