[Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t

[Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t

[Serge Petrenko]

We're using an unsigned int to hold vclock map, but there is no
guarantee that unsigned int will be 4 bytes in size to fit all the 32
vclock components. So use uint32_t instead.
---
No issue, discussed in scope of
https://github.com/tarantool/tarantool/issues/4739
Branch: https://github.com/tarantool/tarantool/tree/sp/vclock-map-refactor

 src/box/vclock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/vclock.h b/src/box/vclock.h
index eb0fb5d8b..9b499513a 100644
--- a/src/box/vclock.h
+++ b/src/box/vclock.h
@@ -82,7 +82,7 @@ enum {
 /** Cluster vector clock */
 struct vclock {
 	/** Map of used components in lsn array */
-	unsigned int map;
+	uint32_t map;
 	/** Sum of all components of vclock. */
 	int64_t signature;
 	int64_t lsn[VCLOCK_MAX];
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t

[Vladislav Shpilevoy]

Hi! Thanks for  the patch!

Consider this diff:

====================
diff --git a/src/box/vclock.h b/src/box/vclock.h
index 9b499513a..a8549d2a9 100644
--- a/src/box/vclock.h
+++ b/src/box/vclock.h
@@ -195,7 +195,7 @@ vclock_copy(struct vclock *dst, const struct vclock *src)
 static inline uint32_t
 vclock_size(const struct vclock *vclock)
 {
-	return __builtin_popcount(vclock->map);
+	return bit_count_u32(vclock->map);
 }
 
 static inline int64_t
@@ -281,7 +281,7 @@ vclock_compare_generic(const struct vclock *a, const struct vclock *b,
 		       bool ignore_zero)
 {
 	bool le = true, ge = true;
-	unsigned int map = a->map | b->map;
+	uint32_t map = a->map | b->map;
 	struct bit_iterator it;
 	bit_iterator_init(&it, &map, sizeof(map), true);
 

Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t

[Serge Petrenko]

Hi! Thanks for the review!

> 5 марта 2020 г., в 23:46, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а):
> 
> Hi! Thanks for  the patch!
> 
> Consider this diff:
> 
> ====================
> diff --git a/src/box/vclock.h b/src/box/vclock.h
> index 9b499513a..a8549d2a9 100644
> --- a/src/box/vclock.h
> +++ b/src/box/vclock.h
> @@ -195,7 +195,7 @@ vclock_copy(struct vclock *dst, const struct vclock *src)
> static inline uint32_t
> vclock_size(const struct vclock *vclock)
> {
> -	return __builtin_popcount(vclock->map);
> +	return bit_count_u32(vclock->map);
> }

Ok. Also fixed for replica id filter size in a separate commit.

diff --git a/src/box/xrow.c b/src/box/xrow.c
index 602049004..5e3cb0709 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1205,7 +1205,7 @@ xrow_encode_subscribe(struct xrow_header *row,
                return -1;
        }
        char *data = buf;
-       int filter_size = __builtin_popcount(id_filter);
+       int filter_size = bit_count_u32(id_filter);
        data = mp_encode_map(data, filter_size != 0 ? 6 : 5);
        data = mp_encode_uint(data, IPROTO_CLUSTER_UUID);
        data = xrow_encode_uuid(data, replicaset_uuid);


> 
> static inline int64_t
> @@ -281,7 +281,7 @@ vclock_compare_generic(const struct vclock *a, const struct vclock *b,
> 		       bool ignore_zero)
> {
> 	bool le = true, ge = true;
> -	unsigned int map = a->map | b->map;
> +	uint32_t map = a->map | b->map;
> 	struct bit_iterator it;
> 	bit_iterator_init(&it, &map, sizeof(map), true);
> 

That’s a shame I missed it. Fixed. The changes are on the branch

--
Serge Petrenko
sergepetrenko@tarantool.org

Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

I think you can just merge the second commit into the
first one. If you don't want, then prepend that commit's
title with a subsystem name.

    xrow: use bit_count_u32() for replica_id filter

instead of

    use bit_count_u32() for replica_id filter

After that LGTM.

Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t

[Serge Petrenko]

> 9 марта 2020 г., в 01:16, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а):
> 
> Hi! Thanks for the fixes!
> 
> I think you can just merge the second commit into the
> first one. If you don't want, then prepend that commit's
> title with a subsystem name.
> 
>    xrow: use bit_count_u32() for replica_id filter
> 
> instead of
> 
>    use bit_count_u32() for replica_id filter
> 

Hi! Changed the commit title.
`use bit_count_u32() for replica_id filter` -> `xrow: use bit_count_u32() for replica_id filter`

> After that LGTM.


--
Serge Petrenko
sergepetrenko@tarantool.org

Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t

[Konstantin Osipov]

* Serge Petrenko <sergepetrenko@tarantool.org> [20/03/05 14:36]:
> We're using an unsigned int to hold vclock map, but there is no
> guarantee that unsigned int will be 4 bytes in size to fit all the 32
> vclock components. So use uint32_t instead.

I think there is a bunch of functions that still use unsigned to
pass the vclock value around.

Besides, if you work on a fix like that, it's best to use a
typedef right away, so that it's easy to change to uint64t or 128
bit integer in the future.

> No issue, discussed in scope of
> https://github.com/tarantool/tarantool/issues/4739
> Branch: https://github.com/tarantool/tarantool/tree/sp/vclock-map-refactor

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t

[Serge Petrenko]

Hi! Thanks for the review.


> 16 марта 2020 г., в 23:31, Konstantin Osipov <kostja.osipov@gmail.com> написал(а):
> 
> * Serge Petrenko <sergepetrenko@tarantool.org> [20/03/05 14:36]:
>> We're using an unsigned int to hold vclock map, but there is no
>> guarantee that unsigned int will be 4 bytes in size to fit all the 32
>> vclock components. So use uint32_t instead.
> 
> I think there is a bunch of functions that still use unsigned to
> pass the vclock value around.

I’ve searched once again and couldn’t find any other references to vclock map
except the ones that were already fixed in the patch.

> 
> Besides, if you work on a fix like that, it's best to use a
> typedef right away, so that it's easy to change to uint64t or 128
> bit integer in the future.

Fixed. I didn’t touch bit_count_u32 though, it feels like a bit of an overkill
to create something like bit_count_vclock_map_t or something.

diff --git a/src/box/vclock.h b/src/box/vclock.h
index a8549d2a9..79e5a1bc0 100644
--- a/src/box/vclock.h
+++ b/src/box/vclock.h
@@ -45,6 +45,8 @@
 extern "C" {
 #endif /* defined(__cplusplus) */
 
+typedef uint32_t vclock_map_t;
+
 enum {
 	/**
 	 * The maximum number of components in vclock, should be power of two.
@@ -82,7 +84,7 @@ enum {
 /** Cluster vector clock */
 struct vclock {
 	/** Map of used components in lsn array */
-	uint32_t map;
+	vclock_map_t map;
 	/** Sum of all components of vclock. */
 	int64_t signature;
 	int64_t lsn[VCLOCK_MAX];
@@ -281,7 +283,7 @@ vclock_compare_generic(const struct vclock *a, const struct vclock *b,
 		       bool ignore_zero)
 {
 	bool le = true, ge = true;
-	uint32_t map = a->map | b->map;
+	vclock_map_t map = a->map | b->map;
 	struct bit_iterator it;
 	bit_iterator_init(&it, &map, sizeof(map), true);
 


>> No issue, discussed in scope of
>> https://github.com/tarantool/tarantool/issues/4739
>> Branch: https://github.com/tarantool/tarantool/tree/sp/vclock-map-refactor
> 
> -- 
> Konstantin Osipov, Moscow, Russia

--
Serge Petrenko
sergepetrenko@tarantool.org

Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t

[Konstantin Osipov]

* Serge Petrenko <sergepetrenko@tarantool.org> [20/03/19 17:40]:
> 
> I’ve searched once again and couldn’t find any other references to vclock map
> except the ones that were already fixed in the patch.

Uhm, sorry, I mixed up vclock map data type and server id data
type, which are dependent. Most of places in the code uses
'unsigned' for server id data type. Perhaps we should introduce a
typedef for it as well.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t

[Kirill Yukhin]

Hello,

On 05 мар 14:36, Serge Petrenko wrote:
> We're using an unsigned int to hold vclock map, but there is no
> guarantee that unsigned int will be 4 bytes in size to fit all the 32
> vclock components. So use uint32_t instead.
> ---
> No issue, discussed in scope of
> https://github.com/tarantool/tarantool/issues/4739
> Branch: https://github.com/tarantool/tarantool/tree/sp/vclock-map-refactor

I've checked your patch into 2.2, 2.3 and master.

--
Regards, Kirill Yukhin