Re: [Tarantool-patches] [PATCH] region: do not rotate slabs in case of single slab purification

[Aleksandr Lyapunov <alyapunov@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5600 bytes --]

I have doubts about this patch. It optimizes one case but degrades another.
Looking at the picture you provided I think about a case:
allocate small xx
allocate huge yyyyyy once. real huge.
truncate the yyyyyy
allocate small yy
truncate that yy
allocate small yy
truncate that yy

Without your patch yy will be allocated near xx, after the second slab was
used for yyyyyy and released then.

With you patch small yy will be allocated in the huge second slab with
a large amount of memory wasted in both slabs and losing data locality.

I think the solution should be more careful:
1)huge slabs should not be reserved in the suggested way.
2)space in the previous slab should be reused if possible.
3)if we only had performance test of the allocator..


> Imagine following situation at the moment of region truncation:
>
>         1 slab      2 slab/HEAD
>       x of s1 used  y of s2 used
>       +---------+   +---------+
> ...->|xx|      |-->|yyyyyy|  |--> NULL
>       |xx|      |   |yyyyyy|  |
>       +---------+   +---------+
>
> And region is going to be truncated by size y (i.g.
> region_truncate(&region, region_used() - y)). After this operation
> second slab will remain empty (slab.used == 0). Currently, this slab is
> returned back to the slab cache, which leads to slab rotation. This may
> not be useful in terms of performance, especially for requests which
> don't abuse region memory (for instance, SQL queries) but do call
> region_truncate() intensively. For example, during transfer tuples to
> ephemeral table truncate() is called for each tuple. What is more,
> truncate is also called on each returned tuple in sql_row_to_port()
> (see runtime_tuple_new()).
>
> Also, taking into consideration revert of 67d7ab4 (see 81dda5b) this may
> lead to memory usage fluctuations: region memory in use before query
> execution and after may not be the same. For instance: total region memory
> in usage is x, but the first slab (head) is empty (since between
> requests region_reset() is called). During query execution
> region_truncate() is called and first slab is put back to the slab
> cache. Now another slab forms head of slab list, but there's no
> guarantee that its size is 0 (since slab's sizes are not zeroed on
> reset). As a result, total region memory may change (decrease) due to
> slab rotation. It is likely to be misleading for users and in particular
> for our testing system: to detect region memory leak cases, we have
> nothing to do but restart Tarantool instance each time before test.
> ---
> Branch:https://github.com/tarantool/small/tree/np/dont-rotate-region-on-truncate
>
>   small/region.c |  2 +-
>   test/region.c  | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/small/region.c b/small/region.c
> index 04476c4..b60ad38 100644
> --- a/small/region.c
> +++ b/small/region.c
> @@ -79,7 +79,7 @@ region_truncate(struct region *region, size_t used)
>   		struct rslab *slab = rlist_first_entry(&region->slabs.slabs,
>   						       struct rslab,
>   						       slab.next_in_list);
> -		if (slab->used > cut_size) {
> +		if (slab->used >= cut_size) {
>   			/* This is the last slab to trim. */
>   			slab->used -= cut_size;
>   			cut_size = 0;
> diff --git a/test/region.c b/test/region.c
> index bd5a4c1..8735a59 100644
> --- a/test/region.c
> +++ b/test/region.c
> @@ -63,6 +63,74 @@ region_test_truncate()
>   	footer();
>   }
>   
> +/**
> + * Make sure that in case cut_size == slab_used (i.e. we are going
> + * to truncate whole slab) slab itself is not put back to the slab
> + * cache. It should be kept in the list and reused instead.
> + */
> +void
> +region_test_truncate_rotate()
> +{
> +	header();
> +
> +	struct region region;
> +
> +	region_create(&region, &cache);
> +
> +	size_t alloc1_sz = 10;
> +	void *ptr = region_alloc(&region, alloc1_sz);
> +
> +	fail_unless(ptr != NULL);
> +
> +	fail_if(region_total(&region) > 5000);
> +	size_t total_before_alloc = region_total(&region);
> +
> +	/*
> +	 * This allocation does not fit in previously
> +	 * allocated slab (default slab size is 4kb), so
> +	 * there's two slabs now in the region list.
> +	 */
> +	size_t alloc2_sz = 10000;
> +	region_alloc(&region, alloc2_sz);
> +	fail_if(total_before_alloc == region_total(&region));
> +
> +	/*
> +	 * Before truncate ('x' is occupied space):
> +	 *
> +	 *    1 slab      2 slab/HEAD
> +	 *   10b used      10kb used
> +	 *  +---------+   +---------+
> +	 *  |xx|      |-->|xxxxxx|  |--> NULL
> +	 *  |xx|      |   |xxxxxx|  |
> +	 *  +---------+   +---------+
> +	 *
> +	 * After truncate:
> +	 *
> +	 *    1 slab      2 slab/HEAD
> +	 *   10b used       0kb used
> +	 *  +---------+   +---------+
> +	 *  |xx|      |-->|         |--> NULL
> +	 *  |xx|      |   |         |
> +	 *  +---------+   +---------+
> +	 *
> +	 */
> +	size_t total_before_truncate = region_total(&region);
> +	region_truncate(&region, alloc1_sz);
> +
> +	/*
> +	 * Whole slab has been purified but it shouldn't be
> +	 * returned back to the slab cache. So that it can be
> +	 * reused for further allocations.
> +	 */
> +	fail_if(total_before_truncate != region_total(&region));
> +	region_alloc(&region, alloc2_sz);
> +	fail_if(total_before_truncate != region_total(&region));
> +
> +	region_free(&region);
> +
> +	footer();
> +}
> +
>   int main()
>   {
>   	quota_init(&quota, UINT_MAX);
> @@ -72,6 +140,7 @@ int main()
>   
>   	region_basic();
>   	region_test_truncate();
> +	region_test_truncate_rotate();
>   
>   	slab_cache_destroy(&cache);
>   }
> -- 
> 2.15.1


[-- Attachment #2: Type: text/html, Size: 5965 bytes --]