Tarantool development patches archive
 help / color / mirror / Atom feed
From: Konstantin Osipov <kostja.osipov@gmail.com>
To: Kirill Yukhin <kyukhin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] small: unite the oscillation cache of all mempools
Date: Thu, 30 Jan 2020 11:34:37 +0300	[thread overview]
Message-ID: <20200130083437.GD631@atlas> (raw)
In-Reply-To: <20200130080223.tyta6lti4xsgwqcg@tarantool.org>

* Kirill Yukhin <kyukhin@tarantool.org> [20/01/30 11:02]:
> Hello,
> 
> Thanks a lot for your answer!
> 
> On 30 янв 00:46, Konstantin Osipov wrote:
> > * Kirill Yukhin <kyukhin@tarantool.org> [20/01/29 13:30]:
> > > On 29 янв 04:48, Maksim Kulis wrote:
> > > > The task is to unite the oscillation cache of all mempools.
> > > > In the function slab_put_with_order() we check if there is any 
> > > > slab of the largest size except the current one, in order to 
> > > > avoid oscillation. 
> > > > https://github.com/tarantool/small/blob/master/small/slab_cache.c#L349
> > > 
> > > Thanks a lot for your patch. Since the change is performance
> > > related, before we start off review process, please provide a
> > > testcase (for tarantool or whatever) which will clearly show
> > > that there're cases where performance is actually increased.
> > > 
> > > We won't accept "nice looking" perf patches.
> > 
> > It's not about performance :)
> 
> Okay.
> 
> > The patch reduces memory fragmentation quite a bit on small-arena
> > systems. Imagine a 1G system with slab_alloc_factor =1.06
> > (default). It can easily have up to 50-100 mempools in slab arena,
> > each holding up one mslab, up to 100-400MB in total. 
> > 
> > The whole point of holding up to such mslab is to avoid mmap()
> > system call (which is costly) in cases like 
> > 
> > for (i = 1..10000)
> >     mslab_alloc()
> >     mslab_free()
> > 
> > This is allocation scenario is called "oscilation".
> > 
> > There is no point in it, because there is already a cached mslab in
> > slab_cache, and it will be used just fine.
> > 
> > Maksim should update the CS comment.
> 
> Text is fine. But we need a test which will clearly show that
> the change will improve something user-visible. If it is not
> then it is refactoring and I doubt we'll take it.

Is this why you accepted transactional DDL without tests and
haven't done any till now?

Nice to see https://github.com/tarantool/tarantool/issues/4349
being shifted around again and again.

It didn't stop you from proclaiming 2.2 stable either...

In the first response you show you don't know what the patch is
about, and in the second top it with double standards for
accepting Mail.Ru and community patches.

Good stewardship indeed...

PS It is of course possible to show that memory fragmentation has
decreased with this patch by allocating a few objects and looking
at memory stats. But such test will be fragile and thus bring more
harm than good. 

-- 
Konstantin Osipov, Moscow, Russia

  reply	other threads:[~2020-01-30  8:34 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-29  1:48 Maksim Kulis
2020-01-29 10:28 ` Kirill Yukhin
2020-01-29 21:46   ` Konstantin Osipov
2020-01-30  8:02     ` Kirill Yukhin
2020-01-30  8:34       ` Konstantin Osipov [this message]
2020-01-30 11:18         ` Alexander Turenko
2020-01-30 12:23           ` Konstantin Osipov
2020-01-30 13:05             ` Alexander Turenko
2020-01-30 14:47               ` Konstantin Osipov
2020-01-30 12:20         ` Kirill Yukhin
2020-01-30 12:36           ` Konstantin Osipov
2020-04-10  8:36 Aleksandr Lyapunov
     [not found] ` <CAPZPwLrNbdCzX7LR+_P8xrZ+unDpw-k2EP1Jj5mAsGOTWz2frA@mail.gmail.com>
2020-04-10 12:44   ` Aleksandr Lyapunov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200130083437.GD631@atlas \
    --to=kostja.osipov@gmail.com \
    --cc=kyukhin@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] small: unite the oscillation cache of all mempools' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox