[Tarantool-patches] [PATCH small 1/1] region: new region_alloc_array, updated alloc_object

[Tarantool-patches] [PATCH small 1/1] region: new region_alloc_array, updated alloc_object

[Vladislav Shpilevoy]

Region's API provided a macro for aligned allocation of an object:
region_alloc_object(). But not less frequent there is a case when
need to allocate an array of objects, also aligned. The patch adds
region_alloc_array() for that.

Also the patch adds an out parameter 'size' for both macros. It
simplifies total size calculation, which is needed almost always,
because total size is included into an error message, if the
allocation fails.

Needed for https://github.com/tarantool/tarantool/issues/4609
---
Branch: http://github.com/tarantool/small/tree/gerold103/region_alloc_array
Issue: https://github.com/tarantool/tarantool/issues/4609

 small/region.h | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/small/region.h b/small/region.h
index bea88c6..df6567d 100644
--- a/small/region.h
+++ b/small/region.h
@@ -386,10 +386,15 @@ struct RegionGuard {
 };
 #endif /* __cplusplus */
 
-#define region_reserve_object(region, T) \
-	(T *)region_aligned_reserve((region), sizeof(T), alignof(T))
-
-#define region_alloc_object(region, T) \
-	(T *)region_aligned_alloc((region), sizeof(T), alignof(T))
+#define region_alloc_object(region, T, size) ({					\
+	*(size) = sizeof(T);							\
+	(T *)region_aligned_alloc((region), sizeof(T), alignof(T));		\
+})
+
+#define region_alloc_array(region, T, count, size) ({				\
+	int _tmp_ = sizeof(T) * (count);					\
+	*(size) = _tmp_;							\
+	(T *)region_aligned_alloc((region), _tmp_, alignof(T));			\
+})
 
 #endif /* INCLUDES_TARANTOOL_SMALL_REGION_H */
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH small 1/1] region: new region_alloc_array, updated alloc_object

[Aleksandr Lyapunov]

Thanks for the patch! See 1 comment below:

On 5/21/20 11:32 PM, Vladislav Shpilevoy wrote:
> Also the patch adds an out parameter 'size' for both macros. It
> simplifies total size calculation, which is needed almost always,
> because total size is included into an error message, if the
> allocation fails.
I don't like the size returning. Even for array allocation it looks 
annoying.
It's too easy to calculate the size, and compilers will omit the second
multiplication with the same args.
For single allocation it looks ugly. It's not even a calculation.

Re: [Tarantool-patches] [PATCH small 1/1] region: new region_alloc_array, updated alloc_object

[Vladislav Shpilevoy]

Hi! Thanks for the review!

> On 5/21/20 11:32 PM, Vladislav Shpilevoy wrote:
>> Also the patch adds an out parameter 'size' for both macros. It
>> simplifies total size calculation, which is needed almost always,
>> because total size is included into an error message, if the
>> allocation fails.
> I don't like the size returning. Even for array allocation it looks annoying.
> It's too easy to calculate the size, and compilers will omit the second
> multiplication with the same args.
> For single allocation it looks ugly. It's not even a calculation.

This is not a matter of how easy it is to calculate a size. This is
a matter of code duplication. Type and count are needed in 2 places:
in region_alloc_array() and then in diag_set(). This is much more ugly,
and occupies significantly more space, when you need to write them
both twice. Compare:

	struct key_def **keys =
		region_alloc_array(&fiber()->gc, typeof(keys[0]),
				   key_count);
	if (keys == NULL) {
		diag_set(OutOfMemory, sizeof(keys[0]) * key_count,
			 "region_alloc_array", "keys");
		return NULL;
	}

And

	size_t bsize;
	struct key_def **keys =
		region_alloc_array(&fiber()->gc, typeof(keys[0]),
				   key_count, &bsize);
	if (keys == NULL) {
		diag_set(OutOfMemory, bsize, "region_alloc_array", "keys");
		return NULL;
	}

In the first option 'keys[0]' and 'count' need to be written 2
times. This is getting worse, when variable name is like 'region_links',
or count is 'def->field_count'.

Worse again it becomes, when size is needed not only in diag, but
also in case of success. For example, to make a memset(0). In this
case it is either triple code duplication, or double code duplication
but with size_t size/bsize variable anyway.

region_alloc_object() got the out parameter to be consistent with
region_alloc_array().

I tried removing the out parameter, and it started looking worse.

Re: [Tarantool-patches] [PATCH small 1/1] region: new region_alloc_array, updated alloc_object

[Vladislav Shpilevoy]

Still need 2 reviews here.

On 21/05/2020 22:32, Vladislav Shpilevoy wrote:
> Region's API provided a macro for aligned allocation of an object:
> region_alloc_object(). But not less frequent there is a case when
> need to allocate an array of objects, also aligned. The patch adds
> region_alloc_array() for that.
> 
> Also the patch adds an out parameter 'size' for both macros. It
> simplifies total size calculation, which is needed almost always,
> because total size is included into an error message, if the
> allocation fails.
> 
> Needed for https://github.com/tarantool/tarantool/issues/4609
> ---
> Branch: http://github.com/tarantool/small/tree/gerold103/region_alloc_array
> Issue: https://github.com/tarantool/tarantool/issues/4609
> 
>  small/region.h | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/small/region.h b/small/region.h
> index bea88c6..df6567d 100644
> --- a/small/region.h
> +++ b/small/region.h
> @@ -386,10 +386,15 @@ struct RegionGuard {
>  };
>  #endif /* __cplusplus */
>  
> -#define region_reserve_object(region, T) \
> -	(T *)region_aligned_reserve((region), sizeof(T), alignof(T))
> -
> -#define region_alloc_object(region, T) \
> -	(T *)region_aligned_alloc((region), sizeof(T), alignof(T))
> +#define region_alloc_object(region, T, size) ({					\
> +	*(size) = sizeof(T);							\
> +	(T *)region_aligned_alloc((region), sizeof(T), alignof(T));		\
> +})
> +
> +#define region_alloc_array(region, T, count, size) ({				\
> +	int _tmp_ = sizeof(T) * (count);					\
> +	*(size) = _tmp_;							\
> +	(T *)region_aligned_alloc((region), _tmp_, alignof(T));			\
> +})
>  
>  #endif /* INCLUDES_TARANTOOL_SMALL_REGION_H */
> 

Re: [Tarantool-patches] [PATCH small 1/1] region: new region_alloc_array, updated alloc_object

[Timur Safin]

: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: Re: [PATCH small 1/1] region: new region_alloc_array, updated
: alloc_object
: 
...
: Worse again it becomes, when size is needed not only in diag, but
: also in case of success. For example, to make a memset(0). In this
: case it is either triple code duplication, or double code duplication
: but with size_t size/bsize variable anyway.
: 
: region_alloc_object() got the out parameter to be consistent with
: region_alloc_array().
: 
: I tried removing the out parameter, and it started looking worse.


Agreed, that if extra macro argument `size` would simplify the code 
then it's a good thing, despite all apparent ugliness. 

Timur

Re: [Tarantool-patches] [PATCH small 1/1] region: new region_alloc_array, updated alloc_object

[Timur Safin]

LGTM

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Thursday, June 4, 2020 12:27 AM
: To: tarantool-patches@dev.tarantool.org; alyapunov@tarantool.org;
: tsafin@tarantool.org
: Subject: Re: [Tarantool-patches] [PATCH small 1/1] region: new
: region_alloc_array, updated alloc_object
: 
: Still need 2 reviews here.
: 
: On 21/05/2020 22:32, Vladislav Shpilevoy wrote:
: > Region's API provided a macro for aligned allocation of an object:
: > region_alloc_object(). But not less frequent there is a case when
: > need to allocate an array of objects, also aligned. The patch adds
: > region_alloc_array() for that.
: >
: > Also the patch adds an out parameter 'size' for both macros. It
: > simplifies total size calculation, which is needed almost always,
: > because total size is included into an error message, if the
: > allocation fails.
: >
: > Needed for https://github.com/tarantool/tarantool/issues/4609
: > ---
: > Branch:
: http://github.com/tarantool/small/tree/gerold103/region_alloc_array
: > Issue: https://github.com/tarantool/tarantool/issues/4609
: >
: >  small/region.h | 15 ++++++++++-----
: >  1 file changed, 10 insertions(+), 5 deletions(-)
: >
: > diff --git a/small/region.h b/small/region.h
: > index bea88c6..df6567d 100644
: > --- a/small/region.h
: > +++ b/small/region.h
: > @@ -386,10 +386,15 @@ struct RegionGuard {
: >  };
: >  #endif /* __cplusplus */
: >
: > -#define region_reserve_object(region, T) \
: > -	(T *)region_aligned_reserve((region), sizeof(T), alignof(T))
: > -
: > -#define region_alloc_object(region, T) \
: > -	(T *)region_aligned_alloc((region), sizeof(T), alignof(T))
: > +#define region_alloc_object(region, T, size) ({
: 	\
: > +	*(size) = sizeof(T);							\
: > +	(T *)region_aligned_alloc((region), sizeof(T), alignof(T));
: 	\
: > +})
: > +
: > +#define region_alloc_array(region, T, count, size) ({
: 	\
: > +	int _tmp_ = sizeof(T) * (count);					\
: > +	*(size) = _tmp_;							\
: > +	(T *)region_aligned_alloc((region), _tmp_, alignof(T));
: 	\
: > +})
: >
: >  #endif /* INCLUDES_TARANTOOL_SMALL_REGION_H */
: >