[tarantool-patches] Re: [PATCH 1/2] Lightweight vclock_create and vclock_copy

[Konstantin Osipov <kostja@tarantool.org>]

* Georgy Kirichenko <georgy@tarantool.org> [19/02/12 17:09]:
>  /**
> @@ -158,6 +159,8 @@ vclock_get(const struct vclock *vclock, uint32_t replica_id)
>  {
>  	if (replica_id >= VCLOCK_MAX)
>  		return 0;

I checked the code and I there is no place which passes arbitrary
replica id. I believe this was added to avoid access to
uninitialized memory in case of corrupt network packet. Now that
vclock_get() becomes a hot path, let's avoid branching by having a
cheaper solution for this problem:

/** Avoid segmentation fault in case of malformed packet. */
replica_id &= VLCOCK_MAX - 1;


> +	if ((vclock->map & (1 << replica_id)) == 0)
> +		return 0;
>  	return vclock->lsn[replica_id];
>  }
>  
> @@ -165,6 +168,8 @@ static inline int64_t
>  vclock_inc(struct vclock *vclock, uint32_t replica_id)
>  {
>  	/* Easier add each time than check. */
> +	if ((vclock->map & (1 << replica_id)) == 0)
> +		vclock->lsn[replica_id] = 0;
>  	vclock->map |= 1 << replica_id;

Since you added a check, please move this assignment inside the
branch.

>  	vclock->signature++;
>  	return ++vclock->lsn[replica_id];
> @@ -173,7 +178,13 @@ vclock_inc(struct vclock *vclock, uint32_t replica_id)
>  static inline void
>  vclock_copy(struct vclock *dst, const struct vclock *src)
>  {
> -	*dst = *src;
> +	if (src->map == 0) {
> +		dst->map = src->map;
> +		dst->signature = src->signature;
> +		return;
> +	}

Why would you ever need this branch? Looks like you can safely
delete it.

> +	unsigned int max_pos = VCLOCK_MAX - bit_clz_u32(src->map);
> +	memcpy(dst, src, offsetof(struct vclock, lsn) + sizeof(*dst->lsn) * max_pos);
>  }

Apart from these minor comments the branch is looking good, thank
you!

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov