Re: [PATCH v2 25/27] attr: store attribute stack in attr_check structure

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 23, 2017, 21:42 UTC
Message-ID: <xmqq37g9qwr2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170123203525.185058-26-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

> The last big hurdle towards a thread-safe API for the attribute system
> is the reliance on a global attribute stack that is modified during each
> call into the attribute system.

The same comment as 22/27 applies here.

It is not an immediate problem we need to solve in the scope of this series, in the sense that a Big Subsystem Lock for the attribute subsystem around git_check_attr() function can make it thread-safe.

But if we want to make it truly threadable without a Big Subsystem Lock, this and the other one would need to become per-thread at least. I think the check_all_attrs scoreboard, which is the topic of 22/27, should become per git_check_attr() invocation (immediately before making a call to collect_some_attrs(), prepare an array with map.size elements and use that as a scoreboard, for example). I do not think we can be sure that the "slimmed down attr stack" 15/27 envisions would help performance without benchmarking, but if it does, then the "attr stack that holds entries that are relevant to the current query" would have to become per <thread, check> pair, as two threads may be executing the same codepath looking for the same set of attributes (i.e. sharing a single attr_check instance), but working on two different parts of a tree structure.

> This patch removes this global stack and instead a stack is stored
> locally in each attr_check instance.  This opens up the opportunity for
> future optimizations to customize the attribute stack for the attributes
> that a particular attr_check struct is interested in.

This is still true. But two threads hitting the same attr_check would make the stack thrash between the paths they are working on to hurt performance once we go multi-threaded.

Perhaps, provided if the "slimmed down attr stack" is indeed a good idea, we should keep the global hashmap that holds everything we read from .gitattributes tree-wide (i.e. as in your v1), _and_ introduce a mechanism to keep the slimmed down version that is relevant to check[] for each thread somehow.