From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id BDB5946970E for ; Thu, 30 Jan 2020 15:16:06 +0300 (MSK) Received: by mail-lf1-f49.google.com with SMTP id t23so2137432lfk.6 for ; Thu, 30 Jan 2020 04:16:06 -0800 (PST) Date: Thu, 30 Jan 2020 15:16:04 +0300 From: Konstantin Osipov Message-ID: <20200130121604.GA11018@atlas> References: <4e734e626aba336b27ec85790747c657d29c0338.1580284383.git.alexander.turenko@tarantool.org> <20200129214122.GB31458@atlas> <20200130072318.zxze72h2proksji5@tkn_work_nb> <20200130082115.GC631@atlas> <20200130110315.gzdi3q37ey2ytvct@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200130110315.gzdi3q37ey2ytvct@tkn_work_nb> Subject: Re: [Tarantool-patches] [small] Revert "Free all slabs on region reset" List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org * Alexander Turenko [20/01/30 14:06]: > > > The Georgy's fix for small keeps linear traversal over allocated slabs > > > on region_reset(). Fixed region_reserve() releases one slab at max per > > > allocation, so this linear traversal will persist for several > > > region_reset() calls in row. > > > > Why do you think region_reset() should traverse over empty slabs? > > Empty slabs should always remain at the end of the list, let's > > stick to this invariant. > > Don't get the point. Do you propose to stop this traverse on a first > unused slabs? AFAIU, there may be slabs with zero slab->used in a > middle. We cannot stop based on `slab->used` value. Let's make sure it's impossible. This is basically what Georgy's patch is doing already. > > region_free()/region_reset() should be (and is in most cases) > > bounded, since it is called as soon as region_used() reaches 128KB. > > So yes, it's a list traversal, but it is done only once in every > > dozen or so requests and is bounded by a list which is a few > > dozens of slabs. > > A list traversal on **each** region_reset(), not once. 20k, not few > dozens. It does not look okay. It is even seen on the benchmark in > #4736 (if uncomment per-batch RPS prints and run it on the patched > version). 20k is caused by the bug, it's not worth considering. Typically it's 128/4k = 32 elements. > > The fix that Georgy made in May was not only about tests. It was > > improving region memory usage (and thus indirectly performance), > > too. > > 1. This need to be verified. > 2. Even if it increases performance on some workloads, it should not > significantly degrade others. > > Small deallocations on during allocation is the good idea. Let's file an > issue against this. > > Linear traversal in region_reset() is the bad one, IMHO. Especially > considering memory access pattern: it is list traversal, not an array. > Especially if we need to traverse a slab N/2 times in average (N is the > list size). At least until the upper limit of the list size is not > proven to be small on any workload. Well, OK. I guess you need time to sort this out. -- Konstantin Osipov, Moscow, Russia