Re: [PATCH v2 12/16] pathspec: create parse_long_magic function

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 12, 2016, 06:19 UTC
Message-ID: <xmqqfultoej1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kbbk4vdW_mbC0riXOf=31V9AQV7zKEh56G+sxjjzAr2-g@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 7 quoted lines

>> I do not see how it would work without further splitting the
>> attr_stack.  I think you made it per check[], but you would further
>> split it per <check, thread> before losing the mutex, no?
>
> Well I have not yet looked into it again, so my memories are
> rusty, but the <check> is read only, such that the answers only
> need to be per thread?

<check> is read-only, so as long as you populate the singleton attr's beforehand, they can be shared across threads. <answer> of course needs an instance per thread, and that is why you have them typically on stack.

The process of filling <answer> by asking for a set of attrs in <check> for one <path> goes roughly like:

 - make sure attr_stack is set up for <path>, namely, the
   info/attributes and .gitattributes files for each leading
   directory are parsed.

 - go over the attr_stack entries and see what entries match <path>,
   and collect the result for <check> in <answer>

Before d90675c151 ("attr: keep attr stack for each check", 2016-11-10), I had only one instance of an attr stack [*1*], but with that commit, you made it per <check>, which is a good move to allow us to optimize by keeping only the attributes relevant to <check> on the attr stack.

But it does not solve the threading issue.

If multiple threads are asking for the same set of attrs (i.e. running the same codepath using the same <check>) but for <path>s in different parts of the working tree (e.g. "git checkout" that is multi-threaded, each thread asking for eol related attributes and checking out different subdirectories), you'd need mutex for correct operation at least, but that won't perform well because you'd end up thrashing the attr stack. You'd need to split attr stack further and make it per (<check>, thread) tuple and you no longer need mutex at that point, but not before that.

[footnote]

*1* This is because the attr subsystem originally wasn't designed to
    be used from multiple threads at the same time hence it was
    sufficient to have a single "currently interested are of the
    directory hierarchy".