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