[prev in list] [next in list] [prev in thread] [next in thread]
List: squid-dev
Subject: Re: [squid-dev] [PATCH] Non-zeroing mempools
From: Kinkie <gkinkie () gmail ! com>
Date: 2015-08-24 10:03:23
Message-ID: CA+Y8hcOXBBOYyumiKXFvkWFebZeaKPQmN1mfabJwUr8AYB6dcQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
>
> >
> >> * 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.
>
Moved offending ones to .cc files. You are right.
>
> - 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.
>
Nod.
>
>
> > * 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.
>
Ok
>
>
> >
> >> * 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.
>
Dropped.
> >
> >> * 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.
>
I like Alex' proposal: it's as simple as it can be, consistent and it seems
to cover all cases nicely.
> >
> >> * 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
>
Why not? It makes sense and I expect it to be self-contained.
I'll try.
> >> * 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.
>
On the plus side, typedefs.h is no longer the beast it used to be, and can
for sure be reduced further. Not doing that here now, but keeping that in
mind.
> More new bits:
>
> in src/fs/ufs/UFSStoreState.h
> * you are changing _queued_write lines but not NULL/nullptr
>
Ok
>
>
> 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.
>
Memory block it is.
>
>
> * AllocatorProxy has tabs in its initializer list.
>
Ok.
> (thats what I'm seeing with a read-thru, haven't tried Alex's script yet).
>
Would it make sense to add that script to the tree? IMO it does if we want
to apply it consistently.
--
Francesco
[Attachment #5 (text/html)]
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span class="">><br> >> * Please make the \
initializer lists one member per line.<br> >> - the auto-formatting wont do \
that for us yet.<br> >> - it greatly helps readability and manual checking \
for missing or<br> >> out-of-order items. We can also then easily add comments \
to lines<br> >> explain odd default values.<br>
>><br>
><br>
> Sure for .cc, not so sure for .h, where compactness helps readability and<br>
> presence and order should be easier to spot. Finally, many missing or<br>
> out-of-order should get caught either by Coverity or gcc.<br>
> Changed for all .cc, not done for .h (where lists are quite shorter)<br>
><br>
<br>
</span>Okay. As we kind of spoke of in IRC it is a "does it look readable" \
call<br> for .h.<br>
<br>
The change to src/URL.h and src/acl/Acl.h are good examples of what I<br>
would like to avoid.<br></blockquote><div><br></div><div>Moved offending ones to .cc \
files. You are right.</div><div> </div><blockquote class="gmail_quote" \
style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
- The orginal single-liner with {} at the end looks fine.<br>
<br>
- A one-per-line also looks fine.<br>
<br>
But with the slightly longer line they have now a partial wrapping in<br>
the set mid-way somewhere makes it hard to \
read.<br></blockquote><div><br></div><div>Nod.</div><div> </div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> <span class=""><br>
<br>
> * in memset() please use 0 integer value not '\0' to set flags.<br>
>> - it helps the compiler know it is safe to optimize up to larger than<br>
>> 8-bit values of 0.<br>
>><br>
><br>
> Ok.<br>
><br>
<br>
</span>I meant just for the ones you were adding. But since this is a patch<br>
scoped at correcting initialization issues I'll accept the ones you are<br>
now changing.<br></blockquote><div><br></div><div>Ok</div><div> </div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> <span class=""><br>
<br>
><br>
>> * src/HttpHeader.cc - revert<br>
>><br>
><br>
> Yes.<br>
><br>
<br>
</span>Now you've got src/ident/AclIdent.h with gratuitous change.<br>
<br>
I like the change itself but its well outside any \
relevance.<br></blockquote><div><br></div><div>Dropped.</div><div> \
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span class=""> ><br>
>> * when adding a new constructor ensure the big-5 operators (ctor, dtor,<br>
>> assign, emplace, emplace-assign) are all present. Even if they use \
"=<br> >> default"<br>
>> - looking at StoreMeta, maybe others.<br>
>><br>
><br>
> StoreMeta is pure-virtual; Also, it has no complex members so emplace<br>
> doesn't add value vs. copy.<br>
> Omitting emplace and implementing the others as protected for \
completeness'<br> > sake.<br>
><br>
<br>
</span>Hmm. We are going to have to decide what to do about cases like that<br>
where Big-3 is more appropriate than Big-5.<br>
<br>
I'm inclined to use emplace =default/=delete anyway just for code clarity.<br>
<br>
Might be worth adding a comment to that effect just to record that<br>
StoreMeta has actually been considered for big-5 \
already.<br></blockquote><div><br></div><div>I like Alex' proposal: it's as \
simple as it can be, consistent and it seems to cover all cases nicely.</div><div> \
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span class=""> ><br>
>> * wordlist missing 'explicit' on the raw-pointer ctor<br>
>> - also other big-5 operators<br>
>><br>
><br>
> wordlist must die; it's a C relic which doesn't belong to squid \
anymore.<br> > Kinda-implemented with a private default destructor (friended \
by<br> > wordlistDestroy) and deleted assignment and copyconstructor. This \
helped<br> > catch a leak in FtpGateway.cc so at least it was used for something \
-even<br> > if it's very ugly.<br>
><br>
<br>
</span>I'm resisting the urge to request this be split to a different patch. \
:-P<br></blockquote><div><br></div><div>Why not? It makes sense and I expect it to be \
self-contained.</div><div>I'll try.</div><div><br></div><div> </div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span class=""> >> * please do one of these:<br>
>><br>
>> + return the XXX to Debugs.h, but it can move up to explain why<br>
>> AllocatorProxy.h is used instead of mem/forward.h<br>
>><br>
><br>
> It doesn't need to. I'm dumb and should have used mem/forward in the \
first<br> > place - you had even told me not too long ago.<br>
<br>
</span>It does. The problem was documented in the old XXX:<br>
<br>
"pulls in typedefs.h and enums.h and globals.h"<br>
<br>
typedefs.h/enums.h/defines.h/globals.h is a nasty nest of recursive<br>
include worms. In particular including them from something widely used<br>
like Debugs.h makes it super nasty doing test builds trying to verify<br>
future shuffling of their contents.<br>
It was weeks of effort to get defines.h out of Debugs.h in the first place.<br>
<br>
Thats why the original XXX existed (as it said). If you just use<br>
mem/forward.h there without the shuffling of FREE typedef and remove<br>
typedefs.h dependency, then the problem is just delayed to a subtle more<br>
nasty issue later.<br>
<br>
Looking at the scope creep that would may involve linkage fixes where<br>
things already depend on typedefs.h includes via mem/forward.h. So I<br>
retract mem/forward.h as an option to be selected. Please do the<br>
AllocatorProxy.h include with a new XXX.<br></blockquote><div><br></div><div>On the \
plus side, typedefs.h is no longer the beast it used to be, and can for sure be \
reduced further. Not doing that here now, but keeping that in \
mind.</div><div><br></div><div> </div><blockquote class="gmail_quote" \
style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> More new \
bits:<br> <br>
in src/fs/ufs/UFSStoreState.h<br>
* you are changing _queued_write lines but not \
NULL/nullptr<br></blockquote><div><br></div><div>Ok</div><div> </div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> <br>
<br>
in src/mem/AllocatorProxy.h:<br>
* please avoid using the term "store" in MEMPROXY_CLASS description.<br>
- it kind of collides with our use of the word for caching.<br>
- "memory" or "memory block" would be better \
here.<br></blockquote><div><br></div><div>Memory block it is.</div><div> \
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> <br>
<br>
* AllocatorProxy has tabs in its initializer \
list.<br></blockquote><div><br></div><div>Ok.</div><div> </div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> (thats what I'm seeing with a read-thru, haven't \
tried Alex's script yet).<br></blockquote><div><br></div><div>Would it make sense \
to add that script to the tree? IMO it does if we want to apply it \
consistently.</div></div><br clear="all"><div><br></div>-- <br><div \
class="gmail_signature"> Francesco</div> </div></div>
[Attachment #6 (text/plain)]
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic