* [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t @ 2020-03-05 11:36 Serge Petrenko 2020-03-05 20:46 ` Vladislav Shpilevoy ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Serge Petrenko @ 2020-03-05 11:36 UTC (permalink / raw) To: v.shpilevoy; +Cc: tarantool-patches 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t 2020-03-05 11:36 [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t Serge Petrenko @ 2020-03-05 20:46 ` Vladislav Shpilevoy 2020-03-06 8:58 ` Serge Petrenko 2020-03-16 20:31 ` Konstantin Osipov 2020-03-20 11:15 ` Kirill Yukhin 2 siblings, 1 reply; 9+ messages in thread From: Vladislav Shpilevoy @ 2020-03-05 20:46 UTC (permalink / raw) To: Serge Petrenko; +Cc: tarantool-patches 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t 2020-03-05 20:46 ` Vladislav Shpilevoy @ 2020-03-06 8:58 ` Serge Petrenko 2020-03-08 22:16 ` Vladislav Shpilevoy 0 siblings, 1 reply; 9+ messages in thread From: Serge Petrenko @ 2020-03-06 8:58 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t 2020-03-06 8:58 ` Serge Petrenko @ 2020-03-08 22:16 ` Vladislav Shpilevoy 2020-03-10 11:23 ` Serge Petrenko 0 siblings, 1 reply; 9+ messages in thread From: Vladislav Shpilevoy @ 2020-03-08 22:16 UTC (permalink / raw) To: Serge Petrenko; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t 2020-03-08 22:16 ` Vladislav Shpilevoy @ 2020-03-10 11:23 ` Serge Petrenko 0 siblings, 0 replies; 9+ messages in thread From: Serge Petrenko @ 2020-03-10 11:23 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t 2020-03-05 11:36 [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t Serge Petrenko 2020-03-05 20:46 ` Vladislav Shpilevoy @ 2020-03-16 20:31 ` Konstantin Osipov 2020-03-19 14:36 ` Serge Petrenko 2020-03-20 11:15 ` Kirill Yukhin 2 siblings, 1 reply; 9+ messages in thread From: Konstantin Osipov @ 2020-03-16 20:31 UTC (permalink / raw) To: Serge Petrenko; +Cc: tarantool-patches, v.shpilevoy * 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t 2020-03-16 20:31 ` Konstantin Osipov @ 2020-03-19 14:36 ` Serge Petrenko 2020-03-19 14:47 ` Konstantin Osipov 0 siblings, 1 reply; 9+ messages in thread From: Serge Petrenko @ 2020-03-19 14:36 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches, Vladislav Shpilevoy 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t 2020-03-19 14:36 ` Serge Petrenko @ 2020-03-19 14:47 ` Konstantin Osipov 0 siblings, 0 replies; 9+ messages in thread From: Konstantin Osipov @ 2020-03-19 14:47 UTC (permalink / raw) To: Serge Petrenko; +Cc: tarantool-patches, Vladislav Shpilevoy * 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t 2020-03-05 11:36 [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t Serge Petrenko 2020-03-05 20:46 ` Vladislav Shpilevoy 2020-03-16 20:31 ` Konstantin Osipov @ 2020-03-20 11:15 ` Kirill Yukhin 2 siblings, 0 replies; 9+ messages in thread From: Kirill Yukhin @ 2020-03-20 11:15 UTC (permalink / raw) To: Serge Petrenko; +Cc: tarantool-patches, v.shpilevoy 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-20 11:15 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-05 11:36 [Tarantool-patches] [PATCH] vclock: refactor vclock map to use type uint32_t Serge Petrenko 2020-03-05 20:46 ` Vladislav Shpilevoy 2020-03-06 8:58 ` Serge Petrenko 2020-03-08 22:16 ` Vladislav Shpilevoy 2020-03-10 11:23 ` Serge Petrenko 2020-03-16 20:31 ` Konstantin Osipov 2020-03-19 14:36 ` Serge Petrenko 2020-03-19 14:47 ` Konstantin Osipov 2020-03-20 11:15 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox