From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 DE91946970E for ; Thu, 30 Jan 2020 14:03:07 +0300 (MSK) Date: Thu, 30 Jan 2020 14:03:16 +0300 From: Alexander Turenko Message-ID: <20200130110315.gzdi3q37ey2ytvct@tkn_work_nb> References: <4e734e626aba336b27ec85790747c657d29c0338.1580284383.git.alexander.turenko@tarantool.org> <20200129214122.GB31458@atlas> <20200130072318.zxze72h2proksji5@tkn_work_nb> <20200130082115.GC631@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200130082115.GC631@atlas> 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: Konstantin Osipov Cc: tarantool-patches@dev.tarantool.org On Thu, Jan 30, 2020 at 11:21:15AM +0300, Konstantin Osipov wrote: > * Alexander Turenko [20/01/30 10:27]: > > 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. > > I think that reserve/reset should give predictable performance and so > > should be constant until region_free() will be called. Ideally it should > > doing something like Georgy made: put one or several unnecessary slabs > > to a slab_cache. Even better would be spread deallocations across > > allocation calls like LuaJIT does: perform more deallocations if there > > are more garbage. > > 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). > > There is no need to complicate it beyond that. It > worked fine for 10 years, after all. I agree that the code should not be much more complex than now. Maybe even should not be changed (however I can say something for sure only after proper benchmarking of different variants on different workloads). > > > > > > BTW, #4750 is only about tests and will be closed soon. If you have > > thoughts about gains we can get from changing of region_*() algorithms, > > please, file a separate issue and describe them. > > > > 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.