[squid-dev] [PATCH] Non-zeroing mempools

[Kinkie]

On Thu, Aug 20, 2015 at 7:11 AM, Amos Jeffries <squid3 at treenet.co.nz> wrote:

> On 20/08/2015 7:28 a.m., Kinkie wrote:
> > Hi all,
> >   the attached patch (a merge request from
> > lp:~kinkie/squid/mempools-nozero) changes MEMPROXY_CLASS so that it
> doesn't
> > zero memory before releasing it to requestors. The constructors of all
> > classes using that feature were checked, so that they fully initialize
> all
> > data members and thus require no zeroing.
> > It also changes Debug::OutStream to use MEMPROXY_CLASS instead of
> > new/derelete, and changes constructors to initializer lists whenever
> > possible and sensible (aka justified by other changes), same for dropping
> > HERE and adopting nullptr.
> > It also changes some code in Makefile.am to use make variables in place
> of
> > full paths.
> > The code has been run-tested (default config only), build-farm-tested and
> > polygraph-tested. It shows a small but noticeable performance improvement
> > in performance (of course, numbers are to be taken with a big grain of
> > salt).
> >
> > When reviewing, please do not focus on formatting issues; if this patch
> > passes review I'll run a source-formatting before merging.
>
>
> * in Makefiles please add the new bits of *_SOURCE lists in alphabetical
> order.
>  - you have several stub_libmem.cc going after stub_M and stub_S*
> filenames.
>

I think I fixed them. It was mentioned in the coverity-fixes thread that it
could be a good idea to do a formatting round sorting all entries in
Makefiles, once the updated standards are finalized. I'd defer any leftover
unsorted entries till then.


> * Please make the initializer lists one member per line.
>  - the auto-formatting wont do that for us yet.
>  - it greatly helps readability and manual checking for missing or
> out-of-order items. We can also then easily add comments to lines
> explain odd default values.
>

Sure for .cc, not so sure for .h, where compactness helps readability and
presence and order should be easier to spot. Finally, many missing or
out-of-order should get caught either by Coverity or gcc.
Changed for all .cc, not done for .h (where lists are quite shorter)

* in memset() please use 0 integer value not '\0' to set flags.
>  - it helps the compiler know it is safe to optimize up to larger than
> 8-bit values of 0.
>

Ok.


> * in MemObject::MemObject() please leave the _reply initialization like
> it was so that the HTTMSGLOCK hack is kept visibly associated with the
> new operation.
>

Ok.


> * You add several "// needs mempool-managed zeroing."
>  - but you say this patch makes the MEMPROXYC_CLASS macro do zero-ing
> already. Those should be reverted.
>

The commets are wrong and based on an outdated analysis of StoreMeta.
Removed the comments.


> * src/HttpHeader.cc - revert
>

Yes.


> * when adding a new constructor ensure the big-5 operators (ctor, dtor,
> assign, emplace, emplace-assign) are all present. Even if they use "=
> default"
>  - looking at StoreMeta, maybe others.
>

StoreMeta is pure-virtual; Also, it has no complex members so emplace
doesn't add value vs. copy.
Omitting emplace and implementing the others as protected for completeness'
sake.


> * wordlist missing 'explicit' on the raw-pointer ctor
>  - also other big-5 operators
>

wordlist must die; it's a C relic which doesn't belong to squid anymore.
Kinda-implemented with a private default destructor (friended by
wordlistDestroy) and deleted assignment and copyconstructor. This helped
catch a leak in FtpGateway.cc so at least it was used for something -even
if it's very ugly.


> * in test-suite/Makefile.am please try to avoid adding LIBMEM_STUBS.
> - DEBUG_SOURCE entries should be enough, and it doesnt matter of extra
> stubs get linked to unit tests.
>

Done in a different way: some unit tests need the full libmem.la; the stub
symbols would override the library, causing failures.


> * please do one of these:
>
>  + return the XXX to Debugs.h, but it can move up to explain why
> AllocatorProxy.h is used instead of mem/forward.h
>

It doesn't need to. I'm dumb and should have used mem/forward in the first
place - you had even told me not too long ago.

Revised patch (with huge context, hence the big size) attached.

Thanks

-- 
    Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150821/28ad927f/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mempools-nozero-v2.patch
Type: application/octet-stream
Size: 93593 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150821/28ad927f/attachment-0001.obj>