From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C78BE46970E for ; Thu, 6 Feb 2020 23:48:23 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <6c6d81d1-d9d2-5248-3edd-f56aefea9e76@tarantool.org> Date: Thu, 6 Feb 2020 21:48:21 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] region: do not rotate slabs in case of single slab purification List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! On 06/02/2020 18:14, Nikita Pettik wrote: > 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(®ion, 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()). I understand the point about perf and slab oscillation, but don't see how is it related to the tests. > 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. Yeah, but your patch does not fix that. It is a bigger problem. Region does not free its slabs even at fiber_gc() if their total size is small enough. So how is this related to memory fluctuations? > 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). How slab size can be 0? You mean 'used' size? In that case it is zeroed in reset. Otherwise I am missing something. > 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. >From what I see, this patch makes region keep slab more often. It does not free them more aggressively or something. So how is it going to fix the slab leak or at least make it less severe? > --- > 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(-)