Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Oct 28, 2018 at 8:52 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
>
> If the field "reason" should always be populated, there is *no*
> reason why we need the "valid" boolean.  They work as a pair to
> realize lazy population of rarely used field.  The lazy evaluation
> technique is used as an optimization for common case, where majority
> of operations do not care if worktrees are locked and if so why they
> are locked, so that only rare operations that do want to find out
> can ask "is this locked and why?" via is_worktree_locked() interface,
> and at that point we lazily find it out by reading "locked" file.
>
> So it is by design that these fields are not always populated, but
> are populated on demand as book-keeping info internal to the API's
> implementation.  It is not "an issue", and changing it is not a
> "fix".

Having fields in a struct that are not populated by a getter function
with no documentation indicating that they are not populated and no
documentation explaining how to populate them is the issue here.

>
> In addition, if we have already checked, then we do not even do the
> same check again.  If in an earlier call we found out that a worktree
> is not locked, we flip the _valid bit to true while setting _reason
> to NULL, so that the next call can say "oh, that's not locked and we
> can tell that without looking at the filesystem again" [*1*].

I clearly misunderstood the use case of the _valid flag, thanks for
pointing it out.

>
> You are forcing the callers of get_worktrees() to pay the cost to
> check, open and read the "why is this worktree locked?" file for all
> worktrees, whether they care if these worktrees are locked or why
> they are locked.  Such a change can be an improvement *ONLY* if you
> can demonstrate that in the current code most codepaths that call
> get_worktrees() end up calling is_worktree_locked() on all worktrees
> anyways.  If that were the case, not having to lazily evaluate the
> "locked"-ness, but always check upfront, would have a simplification
> value, as either approach would be spending the same cost to open
> and read these "locked" files.
>
> But I do not think it is the case.  Outside builtin/worktree.c (and
> you need to admit "git worktree" is a rather rare command in the
> first place, so you shouldn't be optimizing for that if it hurts
> other codepaths), builtin/branch.c wants to go to all worktrees and
> update their HEAD when a branch is renamed (if the old HEAD is
> pointing at the original name, of course), but that code won't care
> if the worktree is locked at all.  I do not think of any caller of
> get_worktrees() that want to know if it is locked and why for each
> and every one of them, and I'd be surprised if that *is* the
> majority, but as a proposer to burden get_worktrees() with this
> extra cost, you certainly would have audited the callers and made
> sure it is worth making them pay the extra cost?
>
> If we are going to change anything around this area, I'd not be
> surprised that the right move is to go in the opposite direction.
> Right now, you cannot just get "is it locked?" boolean answer (which
> can be obtained by a simple stat(2) call) without getting "why is it
> locked?" (which takes open(2) & read(2) & close(2)), and if you are
> planning a new application that wants to ask "is it locked?" a lot
> without having to know the reason, you may want to make the lazy
> evaluation even lazier by splitting _valid field into two (i.e. a
> "do we know if this is locked?" valid bit covers "is_locked" bit,
> and another "do we know why this is locked?" valid bit accompanies
> "locked_reason" string).  And the callers would ask two separate
> questions: is_worktree_locked() that says true or false, and then
> why_worktree_locked() that yields NULL or string (i.e. essentially
> that is what we have as is_worktree_locked() today).  Of course,
> such a change must also be justified with a code audit to
> demonstrate that only minority case of the callers of is-locked?
> wants to know why
>
>
> [Footnote]
>
> *1* The codepaths that want to know if a worktree is locked or not
> (and wants to learn the reason) are so rare and concentrated in
> builtin/worktree.c, and more importantly, they do not need to ask
> the same question twice, so we can stop caching and make
> is_worktree_locked() always go to the filesystem, I think, and that
> may be a valid change _if_ we allow worktrees to be randomly locked
> and unlocked while we are looking at them, but if we want to worry
> about such concurrent and competing uses, we need a big
> repository-wide lock anyway, and it is the least of our problems
> that the current caching may go stale without getting invalidated.
> The code will be racing against such concurrent processes even if
> you made it to go to the filesystem all the time.
>

Basically, I already implemented most of what you're saying. The v2
proposal does force all callers of get_worktrees to check the lock
status, but by calling stat, not open/read/close. That being said
you're right that even forcing them to call stat when most don't care
is imposing an extra cost for no gain. The v2 proposal no longer
caches the lock reason (in fact it removes it from the worktree
struct), since not only do current users have no need to ask for the
lock_reason twice, none of them ask for it twice in the first place.
The v2 proposal provides a standalone function for getting the actual
reason (leaving it up to callers to cache the result if they like).

I'd be up for removing is_locked from the struct as well and making a
separate standalone function for that.

Either way, I do see an issue with the current code that anybody who
wants to know the lock status and/or lock reason of a worktree gets
faced with a confusing, misleading, and opaque piece of code. I see
two possible remedies:

a) Remove these fields from the worktree struct and provide standalone
functions for answering these questions. Pros are that we follow
single responsibility principle with two standalone functions for
doing so, and we make the route for answering these questions less
circuitous (IMO). Cons are that we remove the caching currently in
place since we're no longer storing this info in the struct, but then
again that caching is not currently being used and can be implemented
by callers if they really need it

b) Update the comments in the code to state that lock_reason and
lock_reason_valid are to be considered private fields and to use
is_worktree_locked for populating them. Pros are that no actual code
changes need to be made. Cons are that, IMO, it's still a strange
piece of code in that it's doing some sort of quasi object oriented
stuff in C, and if we can take the opportunity to make the code look a
bit more canonical I think we should, but that's just my 2 cents.

Of course there's also option c, which is that I leave this alone and
just go back to making my worktree atom :)



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux