Re: [PATCH 4/4] fsmonitor: Delay updating state until after split index is merged

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Alex,

On Thu, 19 Oct 2017, Alex Vandiver wrote:

>  extern struct index_state the_index;
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 7c1540c05..4c2668f57 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -49,20 +49,7 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
>  		ewah_free(fsmonitor_dirty);
>  		return error("failed to parse ewah bitmap reading fsmonitor index extension");
>  	}
> -
> -	if (git_config_get_fsmonitor()) {
> -		/* Mark all entries valid */
> -		for (i = 0; i < istate->cache_nr; i++)
> -			istate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;
> -
> -		/* Mark all previously saved entries as dirty */
> -		ewah_each_bit(fsmonitor_dirty, fsmonitor_ewah_callback, istate);
> -
> -		/* Now mark the untracked cache for fsmonitor usage */
> -		if (istate->untracked)
> -			istate->untracked->use_fsmonitor = 1;
> -	}
> -	ewah_free(fsmonitor_dirty);
> +	istate->fsmonitor_dirty = fsmonitor_dirty;

From the diff, it is not immediately clear that fsmonitor_dirty is not
leaked in any code path.

Could you clarify this in the commit message, please?

> @@ -238,6 +225,29 @@ void remove_fsmonitor(struct index_state *istate)
>  
>  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.

In this case, where we trace whether fsmonitor is enabled essentially once
during the life cycle of the current Git command invocation, it may be
okay, but later we perform a trace for every single ie_match_stat() call,
which I think should be guarded to avoid unnecessary code churn in
performance critical code paths (O(N) is pretty good until the constant
factor becomes large).

> +		for (i = 0; i < istate->cache_nr; i++) {
> +			istate->cache[i]->ce_flags |= CE_FSMONITOR_VALID;
> +		}
> +		trace_printf_key(&trace_fsmonitor, "marked all as valid");
> +
> +		/* Mark all previously saved entries as dirty */
> +		trace_printf_key(&trace_fsmonitor, "setting each bit on %p", istate->fsmonitor_dirty);
> +		ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
> +
> +		/* Now mark the untracked cache for fsmonitor usage */
> +		trace_printf_key(&trace_fsmonitor, "setting untracked state");
> +		if (istate->untracked)
> +			istate->untracked->use_fsmonitor = 1;
> +		ewah_free(istate->fsmonitor_dirty);

At this point, please set istate->fsmonitor_dirty = NULL, as it is not
immediately obvious from this patch (or from the postimage of this diff)
that the array is not used later on.

> +	} else {
> +		trace_printf_key(&trace_fsmonitor, "fsmonitor not enabled");
> +	}
> +
>  	switch (git_config_get_fsmonitor()) {
>  	case -1: /* keep: do nothing */
>  		break;
> diff --git a/read-cache.c b/read-cache.c
> index c18e5e623..3b5cd00a2 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -330,6 +330,10 @@ int ie_match_stat(struct index_state *istate,
>  		return 0;
>  	if (!ignore_fsmonitor && (ce->ce_flags & CE_FSMONITOR_VALID))
>  		return 0;
> +	if (ce->ce_flags & CE_FSMONITOR_VALID)
> +		trace_printf_key(&trace_fsmonitor, "fsmon marked valid for %s", ce->name);
> +	if (ignore_fsmonitor)
> +		trace_printf_key(&trace_fsmonitor, "Ignoring fsmonitor for %s", ce->name);

This is the code path I am fairly certain should not be penalized if
tracing is disabled.

Ciao,
Johannes