[Tarantool-patches] [PATCH] do not keep a slab in cache if new allocation does not fit

[Tarantool-patches] [PATCH] do not keep a slab in cache if new allocation does not fit

[Georgy Kirichenko]

Do not allocate a new slab if there is a free slab in a region cache.
If a cached slab is not big enough then free it.

Follow up 67d7ab44ab09df3356929e3692a03321b31f3ebb

Fixes #12

Issue: https://github.com/tarantool/small/issues/12
Branch: https://github.com/tarantool/small/tree/g.kirichenko/gh-12-region-cache

diff --git a/small/region.h b/small/region.h
index d9be176..58481b0 100644
--- a/small/region.h
+++ b/small/region.h
@@ -157,14 +157,20 @@ region_reserve(struct region *region, size_t size)
 		if (size <= rslab_unused(slab))
 			return (char *) rslab_data(slab) + slab->used;
 		/* Try to get a slab from the region cache. */
-		slab = rlist_last_entry(&region->slabs.slabs,
-					struct rslab,
-					slab.next_in_list);
-		if (slab->used == 0 && size <= rslab_unused(slab)) {
-			/* Move this slab to the head. */
+		while ((slab = rlist_last_entry(&region->slabs.slabs,
+						struct rslab,
+						slab.next_in_list))->used == 0) {
 			slab_list_del(&region->slabs, &slab->slab, next_in_list);
-			slab_list_add(&region->slabs, &slab->slab, next_in_list);
-			return (char *) rslab_data(slab);
+			if (size <= rslab_unused(slab)) {
+				/* Move this slab to the head. */
+				slab_list_add(&region->slabs, &slab->slab, next_in_list);
+				return (char *) rslab_data(slab);
+			}
+			/*
+			 * This cached slab could not be used so free it
+			 * and try to use the next one.
+			 */
+			slab_put(region->cache, (struct slab *)slab);
 		}
 	}
 	return region_reserve_slow(region, size);
-- 
2.25.0

Re: [Tarantool-patches] [PATCH] do not keep a slab in cache if new allocation does not fit

[Georgy Kirichenko]

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

Sorry, I forgot an issue and a branch.
Issue: https://github.com/tarantool/tarantool/issues/4736
Branch: https://github.com/tarantool/tarantool/tree/g.kirichenko/gh-4736-region-size-for-gc
On Monday, 27 January 2020 22:39:07 MSK Georgy Kirichenko wrote:
> Do not allocate a new slab if there is a free slab in a region cache.
> If a cached slab is not big enough then free it.
> 
> Follow up 67d7ab44ab09df3356929e3692a03321b31f3ebb
> 
> Fixes #12
> 
> Issue: https://github.com/tarantool/small/issues/12
> Branch:
> https://github.com/tarantool/small/tree/g.kirichenko/gh-12-region-cache
> 
> diff --git a/small/region.h b/small/region.h
> index d9be176..58481b0 100644
> --- a/small/region.h
> +++ b/small/region.h
> @@ -157,14 +157,20 @@ region_reserve(struct region *region, size_t size)
>  		if (size <= rslab_unused(slab))
>  			return (char *) rslab_data(slab) + slab->used;
>  		/* Try to get a slab from the region cache. */
> -		slab = rlist_last_entry(&region->slabs.slabs,
> -					struct rslab,
> -					slab.next_in_list);
> -		if (slab->used == 0 && size <= rslab_unused(slab)) {
> -			/* Move this slab to the head. */
> +		while ((slab = rlist_last_entry(&region->slabs.slabs,
> +						struct rslab,
> +						slab.next_in_list))->used == 0) {
>  			slab_list_del(&region->slabs, &slab->slab, 
next_in_list);
> -			slab_list_add(&region->slabs, &slab->slab, 
next_in_list);
> -			return (char *) rslab_data(slab);
> +			if (size <= rslab_unused(slab)) {
> +				/* Move this slab to the head. */
> +				slab_list_add(&region->slabs, &slab->slab, 
next_in_list);
> +				return (char *) rslab_data(slab);
> +			}
> +			/*
> +			 * This cached slab could not be used so free it
> +			 * and try to use the next one.
> +			 */
> +			slab_put(region->cache, (struct slab *)slab);
>  		}
>  	}
>  	return region_reserve_slow(region, size);


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Tarantool-patches] [PATCH] do not keep a slab in cache if new allocation does not fit

[Konstantin Osipov]

* Georgy Kirichenko <georgy@tarantool.org> [20/01/27 22:39]:
> Do not allocate a new slab if there is a free slab in a region cache.
> If a cached slab is not big enough then free it.
> 
> Follow up 67d7ab44ab09df3356929e3692a03321b31f3ebb
> 

LGTM

Vlad, it would be nice if you do a second review, since this is 
a fix in a stable release.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] do not keep a slab in cache if new allocation does not fit

[Vladislav Shpilevoy]

Hi! Ok, will do after FOSDEM. If it won't be
pushed until that.

On 29/01/2020 22:48, Konstantin Osipov wrote:
> * Georgy Kirichenko <georgy@tarantool.org> [20/01/27 22:39]:
>> Do not allocate a new slab if there is a free slab in a region cache.
>> If a cached slab is not big enough then free it.
>>
>> Follow up 67d7ab44ab09df3356929e3692a03321b31f3ebb
>>
> 
> LGTM
> 
> Vlad, it would be nice if you do a second review, since this is 
> a fix in a stable release.
> 

Re: [Tarantool-patches] [PATCH] do not keep a slab in cache if new allocation does not fit

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 27/01/2020 20:39, Georgy Kirichenko wrote:
> Do not allocate a new slab if there is a free slab in a region cache.
> If a cached slab is not big enough then free it.

Please, describe the problem you are trying to solve. The
text above just narrates the code.

> Follow up 67d7ab44ab09df3356929e3692a03321b31f3ebb
> 
> Fixes #12

Well, the slabs still are not freed in case there was a spike
which allocated lots of slabs, and then only 1-2 are used, and
all allocations fit into the existing slabs.

> 
> Issue: https://github.com/tarantool/small/issues/12
> Branch: https://github.com/tarantool/small/tree/g.kirichenko/gh-12-region-cache
> 
> diff --git a/small/region.h b/small/region.h
> index d9be176..58481b0 100644
> --- a/small/region.h
> +++ b/small/region.h
> @@ -157,14 +157,20 @@ region_reserve(struct region *region, size_t size)
>  		if (size <= rslab_unused(slab))
>  			return (char *) rslab_data(slab) + slab->used;
>  		/* Try to get a slab from the region cache. */
> -		slab = rlist_last_entry(&region->slabs.slabs,
> -					struct rslab,
> -					slab.next_in_list);
> -		if (slab->used == 0 && size <= rslab_unused(slab)) {
> -			/* Move this slab to the head. */
> +		while ((slab = rlist_last_entry(&region->slabs.slabs,
> +						struct rslab,
> +						slab.next_in_list))->used == 0) {
>  			slab_list_del(&region->slabs, &slab->slab, next_in_list);
> -			slab_list_add(&region->slabs, &slab->slab, next_in_list);
> -			return (char *) rslab_data(slab);
> +			if (size <= rslab_unused(slab)) {
> +				/* Move this slab to the head. */
> +				slab_list_add(&region->slabs, &slab->slab, next_in_list);
> +				return (char *) rslab_data(slab);
> +			}
> +			/*
> +			 * This cached slab could not be used so free it
> +			 * and try to use the next one.
> +			 */
> +			slab_put(region->cache, (struct slab *)slab);
>  		}
>  	}
>  	return region_reserve_slow(region, size);
>