Re: [PATCH 32/36] pathspec: allow querying for attributes

On Wed, Nov 9, 2016 at 1:45 AM, Duy Nguyen [off-list ref] wrote:

On Fri, Oct 28, 2016 at 1:29 AM, Junio C Hamano [off-list ref] wrote:

quoted

The reason why I am bringing this up in this discussion thread on
this patch is because I wonder if we would benefit by a similar
"let's not do too involved things and be cheap by erring on the safe
and lazy side" strategy in the call to ce_path_match() call made in
this function to avoid making calls to the attr subsystem.

In other words, would it help the system by either simplifying the
processing done or reducing the cycle spent in preload_thread() if
we could tell ce_path_match() "A pathspec we are checking may
require not just the pattern to match but also attributes given to
the path to satisfy the criteria, but for the purpose of preloading,
pretend that the attribute satisfies the match criteria" (or
"pretend that it does not match"), thereby not having to make any
call into the attribute subsystem at all from this codepath?

The strategy this round takes to make it unnecessary to punt
preloading (i.e. dropping "pathspec: disable preload-index when
attribute pathspec magic is in use" patch the old series had) is to
make the attribute subsystem thread-safe.  But that thread-safety in
the initial round is based on a single Big Attribute Lock, so it may
turn out that the end result performs better for this codepath if we
did not to make any call into the attribute subsystem.

It does sound good and I want to say "yes please do this", but is it
making pathspec api a bit more complex (to express "assume all
attr-related criteria match")? I guess we can have an api to simply
filter out attr-related magic (basically set attr_match_nr back to
zero) then pass a safe (but more relaxing) pathspec to the threaded
code. That would not add big maintenance burden.

So with the current implementation, we already have the shortcut as:

     if (item->attr_match_nr && !match_attrs(name, namelen, item))

i.e. if attr_match_nr is zero, we do not even look at the mutexes and such,
so I am not sure what you intend to say in this email?