On Tuesday, February 12, 2019 10:25:24 PM MSK Konstantin Osipov wrote:
> * 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.
I added this because result of bit_clz_u32(src->map) is undefined in case when 
src->map == 0
> 
> > +	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!