Re: [Tarantool-patches] [small] Revert "Free all slabs on region reset"

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Thu, Jan 30, 2020 at 11:21:15AM +0300, Konstantin Osipov wrote:
> * Alexander Turenko <alexander.turenko@tarantool.org> [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.