[tarantool-patches] Re: [PATCH 1/2] Lightweight vclock_create and vclock_copy
Konstantin Osipov
kostja at tarantool.org
Tue Feb 12 22:25:24 MSK 2019
* Georgy Kirichenko <georgy at 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
More information about the Tarantool-patches
mailing list