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

> 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.
>