Subject: Re: [PATCH 25/27] attr: store attribute stacks in hashmap

From: Junio C Hamano <[email-masked]>
Date: Fri, 13 Jan 2017 13:20:50 -0800

Brandon Williams <[email-masked]> 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.
>
> This patch removes this global stack and instead a stack is retrieved or
> constructed locally.  Since each of these stacks is only used as a
> read-only structure once constructed, they can be stored in a hashmap
> and shared between threads.

Very good.

The reason why the original code used a stack was because it wanted
to keep only the info read from releavant files in-core, discarding
ones from files no-longer relevant (because the traversal switched
to another subdirectory of the same parent directory), to avoid the
memory consumption grow unbounded.  It probably was a premature
"optimization" that we can do without, so keeping everything we have
read so far in a hashmap (which is my understanding of what is going
on in this patch) is probably OK.

I suspect that this hashmap may eventually need to become per
attr_check if we want to follow through the optimization envisioned
by patch 15/27.

Inside fill(), path_matches() is called for the number of match_attr
in the entire attribute stack but it is wasteful to check if the
path matches with the a.u.pat if none of the a.state[] entries talk
about attributes and macros that are eventually get used by the
caller of check_attr().  By introducing a wrapping structure, 15/27
wanted to make sure that we have a place to store a "reduced"
attribute stack that is kept per attr_check that has only entries
from the files that talk about the attributes the particular
attr_check wants to learn about.

I need to think about this a bit more, but I do not offhand think
that it makes future such enhancement to make it per-check harder to
move from a global stack to a global hashmap, i.e. the above is not
an objection to this step.

> One caveat with storing and sharing the stack frames like this is that
> the info stack needs to be treated separately from the rest of the
> attribute stack.  This is because each stack frame holds a pointer to
> the stack that comes before it and if it was placed on top of the rest
> of the attribute stack then this pointer would be different for each
> attribute stack and wouldn't be able to be shared between threads.  In
> order to allow for sharing the info stack frame it needs to be its own
> isolated frame and can simply be processed first to have the same affect
> of being at the top of the stack.

Good.

Thanks.