Re: [PATCH 3/3] reftable/stack: avoid reloading the stack when already locked

On 26/08/19 03:19PM, Karthik Nayak wrote:
> When making modifications to the reftable stack, the stack obtains a
> lock to the list file and removes the lock after the commit phase. Since
> most operations reload the stack to ensure we have the latest state, any
> branched operation during the locked phase could trigger a state reload.
> 
> To prevent data loss due to concurrent writes, state reload is necessary
> right after obtaining the lock. But any reloads after that are just a
> no-op. Now that the struct has access to the lock file status, simply
> skip reloading if the lock is present.

Makes sense.

> Benchmarking with a fixed, non-symbolic target OID shows a modest but
> consistent ~1-2% improvement in clock time for `update-ref` across ref
> counts ranging from 2,000 to 100,000.
> 
> We can see better improvements in the number of syscall counts. On
> master, the number of calls to `newfstatat()` grows linearly with the
> number of refs created. With this patch, the number is now a constant:
> 
>   refcount   master   patch
>   --------   ------   ------
>   1,000      1,059       55
>   5,000      5,059       55
>   10,000     10,059      55
>   20,000     20,059      55
> 
> Reported-by: Jeff King <[email protected]>
> Signed-off-by: Karthik Nayak <[email protected]>
> ---
>  reftable/stack.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/reftable/stack.c b/reftable/stack.c
> index e449af9c03..433a611ed1 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -553,14 +553,21 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
>  
>  /*
>   * Check whether the given stack is up-to-date with what we have in memory.
> + * If skip_if_locked is set skip stack reloading if the stack is currently
> + * locked. Stack reloading must _not_ be skipped right after obtaining the
> + * lock, to check for concurrent updates which may have happened.
> + *
>   * Returns 0 if so, 1 if the stack is out-of-date or a negative error code
>   * otherwise.
>   */
> -static int stack_uptodate(struct reftable_stack *st)
> +static int stack_uptodate(struct reftable_stack *st, int skip_if_locked)
>  {
>  	char **names = NULL;
>  	int err;
>  
> +	if (skip_if_locked && st->list_lock.fd != -1)
> +		return 0;
> +
>  	/*
>  	 * When we have cached stat information available then we use it to
>  	 * verify whether the file has been rewritten.
> @@ -623,7 +630,7 @@ static int stack_uptodate(struct reftable_stack *st)
>  
>  int reftable_stack_reload(struct reftable_stack *st)
>  {
> -	int err = stack_uptodate(st);
> +	int err = stack_uptodate(st, 1);

Ok, this appears to be the only call site where is actually want to skip
if there is a lock present. Could we instead just not invoke
`stack_uptodate()` in such cases? That way we don't have to change its
function signature and can leave all other existing call sites alone.

-Justin