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

[Amos Jeffries]

On 22/08/2015 4:43 a.m., Kinkie wrote:
> 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.
> 

You did that, and then some.

The ones you are now just shuffling around can be reverted until the
makefile cleanup project happens. I meant just for the new entries being
added to some _SOURCES lists.



> 
>> * 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)
> 

Okay. As we kind of spoke of in IRC it is a "does it look readable" call
for .h.

The change to src/URL.h and src/acl/Acl.h are good examples of what I
would like to avoid.

- The orginal single-liner with {} at the end looks fine.

- A one-per-line also looks fine.

But with the slightly longer line they have now a partial wrapping in
the set mid-way somewhere makes it hard to read.


> * 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.
> 

I meant just for the ones you were adding. But since this is a patch
scoped at correcting initialization issues I'll accept the ones you are
now changing.


> 
>> * src/HttpHeader.cc - revert
>>
> 
> Yes.
> 

Now you've got src/ident/AclIdent.h with gratuitous change.

I like the change itself but its well outside any relevance.


> 
>> * 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.
> 

Hmm. We are going to have to decide what to do about cases like that
where Big-3 is more appropriate than Big-5.

I'm inclined to use emplace =default/=delete anyway just for code clarity.

Might be worth adding a comment to that effect just to record that
StoreMeta has actually been considered for big-5 already.


> 
>> * 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.
> 

I'm resisting the urge to request this be split to a different patch. :-P


> 
> 
>> * 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.

It does. The problem was documented in the old XXX:

 "pulls in typedefs.h and enums.h and globals.h"

typedefs.h/enums.h/defines.h/globals.h is a nasty nest of recursive
include worms. In particular including them from something widely used
like Debugs.h makes it super nasty doing test builds trying to verify
future shuffling of their contents.
 It was weeks of effort to get defines.h out of Debugs.h in the first place.

Thats why the original XXX existed (as it said). If you just use
mem/forward.h there without the shuffling of FREE typedef and remove
typedefs.h dependency, then the problem is just delayed to a subtle more
nasty issue later.

Looking at the scope creep that would may involve linkage fixes where
things already depend on typedefs.h includes via mem/forward.h. So I
retract mem/forward.h as an option to be selected. Please do the
AllocatorProxy.h include with a new XXX.



More new bits:

in src/fs/ufs/UFSStoreState.h
* you are changing _queued_write lines but not NULL/nullptr


in src/mem/AllocatorProxy.h:
* please avoid using the term "store" in MEMPROXY_CLASS description.
 - it kind of collides with our use of the word for caching.
 - "memory" or "memory block" would be better here.


* AllocatorProxy has tabs in its initializer list.



(thats what I'm seeing with a read-thru, haven't tried Alex's script yet).

Amos