[squid-dev] [PATCH] mempools-nozero: turn StoreEntry into MEMPROXY_CLASS

[Kinkie]

kkjuOn Aug 28, 2015 7:24 PM, "Alex Rousskov" <
rousskov at measurement-factory.com> wrote:

> On 08/28/2015 09:02 AM, Kinkie wrote:
>
> > On Thu, Aug 27, 2015 at 9:28 PM, Alex Rousskov wrote:
> >
> >     On 08/27/2015 08:58 AM, Kinkie wrote:
> >     >   this patch turns StoreEntry into MEMPROXY_CLASS, getting rid of
> the
> >     > default constructors.
> >     >   In the process, it turns hash_link into a class, giving it a
> default
> >     > constructor in order to support nonzeroing pools.
> >     >   The old custom allocator also gave a nonstandard chunk size; that
> >     > tweak should not be necessary anymore.
>
>
> >     Can you explain why that tweak should not be necessary any more?
>
>
> > It was discussed on IRC (log edited for clarity, date is 2015-08-20)
>
> Does not seem like it was _explained_ though.
>
> Our vague understanding of the original code seems to revolve around
> something like "larger chunk size was specified to allocate large number
> of StoreEntry objects efficiently". It is not clear to me why allocating
> large number of StoreEntry objects efficiently no longer requires larger
> chunks.
>
> It is strange that your proposed commit message claims that something
> should not be needed any more but we do not actually know whether it is
> needed or not. If this goes in, I would recommend clearly stating that
> your are changing something potentially major that you _hope_ has no
> negative performance effects.
>
>
> > <yadi> kinkie: I think this calls for an experimental targeted
> > conversion with lots of tests on what behaviour is affected.
>
>
> I would agree in general, but I doubt you can run relevant tests in this
> case. Perhaps there a way to avoid this particular part of the change
> and keep the StoreEntry chunk sizes the same as they are today?
>

Unfortunately not if we want to proceed with MEMPROXY_CLASS conversion,
which doesn't expose that method, and MEMPROXY_CLASS is with this branch
the easiest (I would dare to say standard) way to go to non-zeroing pools.

I've tried to figure out the code some more; comments highlight that it's
an optimization feature, which is only effective with the chunked allocator
- it is otherwise a nop.
I believe this hint its is meant to try and alter the behavior of the
chunked allocator - in a roundabout way - hinting that the objects in the
pool are important and thus to try and make that particular object pool
less sensitive to memory pressure, by ensuring that if even a single
StoreEntry can be allocated, there will be at least (chunk_size/obj_size
-1) available memory areas until all of them have been released.

Maybe we could test the effects of this optimization by setting a low
memory_pools_limit, but then whatever gap in performance would probably be
dwarfed by fallback to malloc when allocating other objects due to the
pressure.

In other words, I suspect that whatever is the optimization value of chunk
size setting, it is dwarfed by other effects and is speculative; at the
same time we have a sure gain by not initializing objects twice (one when
zeroing the pool, and then in the constructor).
For this reason, I would like to go ahead with the merge, I agree it's a
good idea to isolate this patch into a separate commit, and to mark it as
potentially performance-impacting in the commit message.

Please let me know if you agree.

  Kinkie
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150830/7158aed9/attachment-0001.html>