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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 25, 2017, 20:14 UTC
Message-ID: <xmqq1svqc2ww.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170125195745.GA83343@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 7 quoted lines

>> In my mind, the value of having a constant check_attr is primarily
>> that it gives us a stable pointer to serve as a hashmap key,
>> i.e. the identifier for each call site, in a later iteration.
>
> We didn't really discuss this notion of having the pointer be a key into
> a hashmap, what sort of information are you envisioning being stored in
> this sort of hashmap?

The "entries relevant to this attr_check() call, that is specific to the <check_attr instance, the thread> tuple" (aka "what used to be called the global attr_stack") we discussed would be the primary example. A thread is likely be looping in a caller that has many paths inside a directory, calling a function that has a call to attr_check() for each path. Having something that can use to identify the check_attr instance in a stable way, even when the inner function is called and returns many times, would allow us to populate the "attr stack" just once for the thread when it enters a directory for the first time (remember, another thread may be executing the same codepath, checking for paths in a different directory) and keep using it. There may be other mechanisms you can come up with, so I wouldn't say it is the only valid way, but it is a way. That is why I said:

>> But all of the above comes from my intuition, and I'll very much
>> welcome to be proven wrong with an alternative design, or better
>> yet, a working code based on an alternative design ;-).

near the end of my message.

> One issue I can see with this is that the
> functions which have a static attr_check struct would still not be thread
> safe if the initialization of the structure isn't surrounded by a mutex
> itself. ie

Yes, that goes without saying. That is why I suggested Stefan to do not this:

Show 11 quoted lines

> static struct attr_check *check;
>
> if (!check)
>   init(check);
>
> would need to be:
>
> lock()
> if (!check)
>   init(check);
> unlock();

but this:

	static struct attr_check *check;
	init(&check);

and hide the lock/unlock gymnastics inside the API. I thought that already was in what you inherited from him and started your work on top of?