From: Ben Peart Date: Mon, 23 Oct 2017 12:36:19 GMT Subject: Re: [PATCH 4/4] fsmonitor: Delay updating state until after split index is merged Message-ID: <541ed27f-3219-fe0a-beb6-99ecb4ed36da@gmail.com> In-Reply-To: On 10/23/2017 5:57 AM, Johannes Schindelin wrote: > Hi Peff, > > On Fri, 20 Oct 2017, Jeff King wrote: > >> On Fri, Oct 20, 2017 at 03:16:20PM +0200, Johannes Schindelin wrote: >> >>>> void tweak_fsmonitor(struct index_state *istate) >>>> { >>>> + int i; >>>> + >>>> + if (istate->fsmonitor_dirty) { >>>> + /* Mark all entries valid */ >>>> + trace_printf_key(&trace_fsmonitor, "fsmonitor is enabled; cache is %d", istate->cache_nr); >>> >>> Sadly, a call to trace_printf_key() is not really a noop when tracing is >>> disabled. The call to trace_printf_key() hands off to trace_vprintf_fl(), >>> which in turn calls prepare_trace_line() which asks trace_want() whether >>> we need to trace, which finally calls get_trace_fd(). This last function >>> initializes a trace key if needed, and this entire call stack takes time. >> >> It seems like we could pretty easily turn noop traces into a trivial >> conditional, like: >> >> diff --git a/trace.h b/trace.h >> index 179b249c59..c46b92cbde 100644 >> --- a/trace.h >> +++ b/trace.h >> @@ -80,8 +80,11 @@ extern void trace_performance_since(uint64_t start, const char *format, ...); >> #define trace_printf(...) \ >> trace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__) >> >> -#define trace_printf_key(key, ...) \ >> - trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__) >> +#define trace_printf_key(key, ...) do { \ >> + if (!key->initialized || key->fd) \ >> + trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key, __VA_ARGS__) \ >> +} while(0) >> + >> >> #define trace_argv_printf(argv, ...) \ >> trace_argv_printf_fl(TRACE_CONTEXT, __LINE__, argv, __VA_ARGS__) >> >> >> (OK, that's got an OR, but if we are really pinching instructions we >> could obviously store a single "I've been initialized and am disabled" >> flag). > > I'd really like that. > >> I don't have an opinion one way or the other on these particular >> messages, but in general I'd like to see _more_ tracing in Git, not >> less. I've often traced Git with a debugger or other tools like perf, >> but there's real value in the author of code annotating high-level >> logical events. > > Yes, I like that idea, too: to add more tracing. > > Thanks, > Dscho > Two big thumbs up. I find turning on tracking very informative as I can better see the flow of execution and sometimes am surprised by what I discover. :) I've often added additional tracing while working on a feature only to remove it before submitting the patch because of the performance overhead. Being able to leave that in would be very helpful.