[tarantool-patches] Re: [PATCH 1/2] Lightweight vclock_create and vclock_copy
Георгий Кириченко
georgy at tarantool.org
Tue Feb 12 22:37:17 MSK 2019
On Tuesday, February 12, 2019 10:25:24 PM MSK Konstantin Osipov wrote:
> * 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.
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!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20190212/0f48a860/attachment.sig>
More information about the Tarantool-patches
mailing list