Re: [squid-dev] Fwd: [PATCH] for loops modernization

[Amos Jeffries]

On 16/03/2017 3:43 a.m., Adam Majer wrote:
> On 03/15/2017 03:17 PM, Amos Jeffries wrote:
>> Theoretically range-for loops should allow multi-threaded CPU to run
>> those loops a bit faster. If that can be demonstrated using a tool like
>> polygraph you have a good argument for a patch containing that change to
>> go in as a pure performance change.
> 
> No. That would break many many things. There are special directives that
> allow this to happen with things like OpenMPI compilers, but that's not
> what we are talking about here.
> 
> And theoretically, if you blindly allow compilers to optimize loops like
> that, you are just as likely to introduce hardware stalls that will
> result in slower execution of the overall loop. The only way to look at
> these,
> 
> for (TYPE _i : _c )
> 
> is syntactic sugar.
> 
> 
> Best regards,
> - Adam
> 
> PS. And if you are talking about vertorization of these loops, that
> already happens with regular loops. See,
> 
> https://gcc.gnu.org/projects/tree-ssa/vectorization.html

I mean tricks like compiler with CPU-specific knowledge being able to
emit assembly that helps pre-fetch the address pointers for all objects
in the container, and/or if it can prove the objects are read-only can
have hyper-threads pre-load the container contents into L1/L2 cache in
time for the main thread to run the business logic faster without much
loading delays.

AFAIK the range-for does allow certain code flow guarantees (like
full-length container iteration) being known without any analysis. So
not completely syntactic sugar. Yes compiler could do the same with
traditional loops, but only after extra analysis which might be turned off.

I'm not sure if thus would have any visible effect at all. We might be
unlucky in that these things are not supportable, or the data sizes
Squid handles blow the benefits away. Thus the request for proof.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Fwd: [PATCH] for loops modernization

[Amos Jeffries]

On 16/03/2017 5:03 a.m., Adam Majer wrote:
> On 03/15/2017 04:38 PM, Alex Rousskov wrote:
>> On 03/15/2017 06:57 AM, Adam Majer wrote:
>>> If you really want to fix this, avoid `auto` in code people actually
>>> want to maintain. The only place it is actually useful is in heavily
>>> templated code and that is the main reason it was introduced. It was not
>>> introduced so you can avoid typing the name of POS (plain old
>>> structures) or types or classes.
>>

IMO, and since I'm mostly the one doing these changes will probably be
the usual way going forward - is that if the type being used is complex
enough to actually have/need a typedef to make the type name simpler,
then auto is appropriate.

Specifically the way we historically have acronyms as typedef labels
means one often has to lookup or pay specific attention to what the
definition was anyway. So looking up an auto is no worse and at least
auto does not pretend to be a class name somewhere.

I also find (so far, and specifically with Squid code) that with
range-for loops the name of the variable/getter function/method on the
RHS of the range-for first line is usually more important to know how
its best used than exact iterator type. If that name is not
self-documenting what the object being handled is we have a larger
problem to fix first which may change whether range-for is appropriate
at all.

Other uses of auto are context-specific, but yes it can be less useful
than one would extect from the hype.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] Build failed in Jenkins: template-full-matrix » clang,d-ubuntu-wily #305

[noc]

See 


--
[...truncated 954.98 KB...]
make[5]: Leaving directory 
'
Making dvi in NIS
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in POP3
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in RADIUS
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in SMB
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in fake
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in getpwnam
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi-am'.
make[5]: Leaving directory 
'
make[4]: Leaving directory 
'
Making dvi in digest
make[4]: Entering directory 
'
Making dvi in file
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi-am'.
make[5]: Leaving directory 
'
make[4]: Leaving directory 
'
Making dvi in negotiate
make[4]: Entering directory 
'

Re: [squid-dev] Rock store stopped accessing discs

[Alex Rousskov]

On 03/15/2017 11:57 AM, Heiler Bemerguy wrote:

> Commented out all the max-swap-rate and swap-timeout. Guess what? No
> more errors on cache.log..

> It seems the code that "shapes" the rockstore I/Os is kinda buggy.

I have not checked the code, but I suspect that

* You see no timeout errors because you disabled timeouts.

* You see no I/O delay warnings because you disabled I/O delays.

Said that, I would not be surprised if disabling max-swap-rate reduces
the probability of an I/O message getting stuck in the queue because
without artificial message delays, the probability of a kid dying with a
pending message is naturally lower.

There are probably some bugs in the code even when no kids are dying,
but there is currently no information warranting an investigation AFAICT.

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] To make squid works in snap world.

[Alex Rousskov]

On 03/15/2017 11:03 AM, Gary Wang wrote:
> On Thu, Mar 16, 2017 at 12:33 AM, Alex Rousskov wrote:
>> On 03/15/2017 03:24 AM, Gary Wang wrote:
>> > Regarding the confinement of usage of shared memory in snap world,
>> >Please take a look at the bug
>> >https://bugs.launchpad.net/snappy/+bug/1653955

>> The above bug description appears to imply that snap allows
>> /dev/shm/snap.@{SNAP_NAME}.* names, which are not the names used in your
>> prior examples (IIRC). The sem.* pattern you used confused reviewers
>> into thinking that you are trying to solve the wrong problem. The actual
>> problem you are trying to solve (AFAICT) is valid and is not about the
>> "sem." prefix but about the "snap." prefix.



> A: Yes, We finally see it the same way.

IIRC, until now, there was no disagreement among you and me regarding
this aspect of your submission. You need to convince Amos that you are
solving the right problem.


>In snap world, only the following name pattern is allowed
> /dev/shm/sem.snap.@{SNAP_NAME}.*
>not 
> /dev/shm/snap.@{SNAP_NAME}.*


That statement appears to contradict bug/1653955 description and, if
true, sounds like a snap bug, but, fortunately, it has no significant
effect on the proposed argument sketch. Here is a revised version:

1. Snap does not allow arbitrary names in /dev/shm/.
2. Snap allows names matching /dev/shm/.*
3. I need to make Squid names for /dev/shm/ files configurable
   so that they can be forced to match the snap-allowed pattern.

Alex.
P.S. If there is something you can do to your email application to fix
quoting in the plain text version of the messages it sends, please do so.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Rock store stopped accessing discs

[Heiler Bemerguy]

Hello list,

I've made a simple test here.

cache_dir rock /cache2 11 min-size=0 max-size=65536
#max-swap-rate=150 swap-timeout=360
cache_dir rock /cache3 11 min-size=65537 max-size=262144
#max-swap-rate=150 swap-timeout=380
cache_dir rock /cache4 11 min-size=262145
#max-swap-rate=150 swap-timeout=500

Commented out all the max-swap-rate and swap-timeout. Guess what? No 
more errors on cache.log..


before:
root@proxy:/var/log/squid# cat cache.log.2 |grep overflow |wc -l
53

now:
root@proxy:/var/log/squid# cat cache.log |grep overflow |wc -l
1

before:
root@proxy:/var/log/squid# cat cache.log.2 |grep "7.00s timeout" |wc -l
86

now:
root@proxy:/var/log/squid# cat cache.log |grep "7.00s timeout" |wc -l
0

It seems the code that "shapes" the rockstore I/Os is kinda buggy. 
Version 4.0.18


--
Atenciosamente / Best Regards,

Heiler Bemerguy
Network Manager - CINBESA
55 91 98151-4894/3184-1751


Em 07/03/2017 20:26, Alex Rousskov escreveu:

On 03/07/2017 01:08 PM, Heiler Bemerguy wrote:


Some log from right now...

Here is my analysis:


15:53:05.255| ipc/Queue.h findOldest: peeking from 7 to 6 at 1

Squid worker (kid6) is looking at the queue of disker (kid7) responses.
There is just one response in the queue.



IpcIoFile.cc canWait: cannot wait: 1136930911 oldest: ipcIo6.381049w7

Squid worker is trying to estimate whether it has enough time to queue
(and eventually perform) more disk I/O. The expected wait is
1'136'930'911 milliseconds or ~13 days. That is longer than the
configured cache_dir swap-timeout (a few hundred milliseconds) so Squid
refuses to queue this disk I/O request:


store_io.cc storeCreate: no swapdirs for e:=sw1p2RDV/0x206e17d0*4


The same story happens in your other log snippets AFAICT:


15:53:05.302| ipc/Queue.h findOldest: peeking from 8 to 6 at 1
IpcIoFile.cc canWait: cannot wait: 1136930958 oldest: ipcIo6.153009r8
store_io.cc storeCreate: no swapdirs for e:=msw1DV/0x3a4d5870*2

and


15:53:05.318| ipc/Queue.h findOldest: peeking from 7 to 6 at 1
IpcIoFile.cc canWait: cannot wait: 1136930974 oldest: ipcIo6.381049w7
store_io.cc storeCreate: no swapdirs for e:m381432=w1p2DV/0x3ace6740*4

and


15:53:05.793| ipc/Queue.h findOldest: peeking from 7 to 6 at 1
IpcIoFile.cc canWait: cannot wait: 1136931448 oldest: ipcIo6.381049w7
store_io.cc storeCreate: no swapdirs for e:=sw1V/0x206e17d0*1


Focusing on one disker (kid7), we can see that the oldest response does
not change: It is always ipcIo6.381049w7. This stuck response results in
gradual increment of the expected wait time with every check, matching
wall clock time increment:


15:53:05.255| cannot wait: 1136930911
15:53:05.318| cannot wait: 1136930974
15:53:05.793| cannot wait: 1136931448

These stuck disker responses probably explain why your disks do not
receive any traffic. It is potentially important that both disker
responses shown in your logs got stuck at approximately the same
absolute time ~13 days ago (around 2017-02-22, give or take a day;
subtract 1136930911 milliseconds from 15:53:05.255 in your Squid time
zone to know the "exact" time when those stuck requests were queued).

How can a disker response get stuck? Most likely, something unusual
happened ~13 days ago. This could be a Squid bug and/or a kid restart.

* Do all currently running Squid kid processes have about the same start
time? [1]

* Do you see ipcIo6.381049w7 or ipcIo6.153009r8 mentioned in any old
non-debugging messages/warnings?

[1]
http://stackoverflow.com/questions/5731234/how-to-get-the-start-time-of-a-long-running-linux-process


Thank you,

Alex.



___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] To make squid works in snap world.

[Gary Wang]

On Thu, Mar 16, 2017 at 12:33 AM, Alex Rousskov <
rouss...@measurement-factory.com> wrote:

> On 03/15/2017 03:24 AM, Gary Wang wrote:
> > Regarding the confinement of usage of shared memory in snap world,
> >Please take a look at the bug
> >https://bugs.launchpad.net/snappy/+bug/1653955
>
> The above bug is about semaphores. Squid does not use semaphores (our
> implementation of shared memory objects is lockless). Squid uses shared
> memory segments created by shm_open(), not sem_open() system calls.
> A: Sorry about that, I need to make it clear at this point.



> The above bug description appears to imply that snap allows
> /dev/shm/snap.@{SNAP_NAME}.* names, which are not the names used in your
> prior examples (IIRC). The sem.* pattern you used confused reviewers
> into thinking that you are trying to solve the wrong problem. The actual
> problem you are trying to solve (AFAICT) is valid and is not about the
> "sem." prefix but about the "snap." prefix.
> A: Yes, We finally see it the same way. Just a little correction.

In snap world, only the following name pattern is allowed
/dev/shm/sem.snap.@{SNAP_NAME}.*
   not

> /dev/shm/snap.@{SNAP_NAME}.*



>
> >And Jamie's reply can be fuond in snapcraft maillist
> >  https://www.mail-archive.com/snapcraft@lists.snapcraft.io/msg02465.html
>
> Again, that feels like an irrelevant piece of information here because
> it is specific to semaphores that Squid does not use.
>
>
> If I am right, then your argument should be something like this:
>
> 1. Snap does not allow arbitrary names in /dev/shm/.
> 2. Snap allows names matching /dev/shm/snap.@{SNAP_NAME}.* (and others)

3. I need to make Squid names for /dev/shm/ files configurable
>so that they can be forced to match one of the snap-allowed patterns.
> A: Right, but again snap-allowed name pattern

 /dev/shm/snap.@{SNAP_NAME}.*
   should be
 /dev/shm/sem.snap.@{SNAP_NAME}.*

>
> Please note that it is theoretically possible that OS shm_open()
> implementation (in Squid context) creates some secret temporary files
> just like sem_open() does in Python context. That would mean that other
> names/patterns may be in play here as well. However, since you know that
> your patch "works", my argument sketch above remains valid even if there
> are other OS-created names that we do not know about.
>
>
> HTH,
>
> Alex.
>
>


-- 
Br
Gary.Wzl
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] To make squid works in snap world.

[Alex Rousskov]

On 03/15/2017 03:24 AM, Gary Wang wrote:
> Regarding the confinement of usage of shared memory in snap world,
>Please take a look at the bug
>https://bugs.launchpad.net/snappy/+bug/1653955

The above bug is about semaphores. Squid does not use semaphores (our
implementation of shared memory objects is lockless). Squid uses shared
memory segments created by shm_open(), not sem_open() system calls.

The above bug description appears to imply that snap allows
/dev/shm/snap.@{SNAP_NAME}.* names, which are not the names used in your
prior examples (IIRC). The sem.* pattern you used confused reviewers
into thinking that you are trying to solve the wrong problem. The actual
problem you are trying to solve (AFAICT) is valid and is not about the
"sem." prefix but about the "snap." prefix.


>And Jamie's reply can be fuond in snapcraft maillist  
>  https://www.mail-archive.com/snapcraft@lists.snapcraft.io/msg02465.html

Again, that feels like an irrelevant piece of information here because
it is specific to semaphores that Squid does not use.


If I am right, then your argument should be something like this:

1. Snap does not allow arbitrary names in /dev/shm/.
2. Snap allows names matching /dev/shm/snap.@{SNAP_NAME}.* (and others)
3. I need to make Squid names for /dev/shm/ files configurable
   so that they can be forced to match one of the snap-allowed patterns.


Please note that it is theoretically possible that OS shm_open()
implementation (in Squid context) creates some secret temporary files
just like sem_open() does in Python context. That would mean that other
names/patterns may be in play here as well. However, since you know that
your patch "works", my argument sketch above remains valid even if there
are other OS-created names that we do not know about.


HTH,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Fwd: [PATCH] for loops modernization

[Adam Majer]

On 03/15/2017 04:38 PM, Alex Rousskov wrote:
> On 03/15/2017 06:57 AM, Adam Majer wrote:
>> If you really want to fix this, avoid `auto` in code people actually
>> want to maintain. The only place it is actually useful is in heavily
>> templated code and that is the main reason it was introduced. It was not
>> introduced so you can avoid typing the name of POS (plain old
>> structures) or types or classes.
> 
> I disagree on all counts. Auto is much more nuanced. The web has a lot
> more pro- and cons- arguments about auto, including advice from C++
> gurus. For example:
> https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/

I would disagree with you on these accounts and with Mayers (if that is
actually his position) auto has its place and that place is generally
when dealing with templates, and especially in templates where types are
not fixed in the definition. So auto simplifies boilerplate. It's a very
good feature.

Sure, you are more than welcome to replace every instance of type with
auto, but don't be very surprised when few weeks from now you are
looking at a piece of code and want to know what structure something
comes from and then tracing things through the code just because you
wanted to use auto instead of `Foo`.

But arguing this kind of reminds of trying to argue with someone that
for loops are not dead because you can now do everything with
std::algorithm and lambdas. It's not very productive.

C++ is a statically typed language and auto does not make that go away.

> In my personal experience, going against Sutter and Mayers advice
> usually ends up in long-term regrets. They are sometimes wrong, but it
> is usually much easier to fix their wrongs than mine. (Just like with
> Squid or any other development, it is often about making good mistakes
> versus the bad ones.)

Let's just say just because someone is famous and knows a few things
does not imply others don't know things. I've learned on my own skin
that hiding types can cause serious problems, but maybe it's just me.

Cheers,
Adam

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Fwd: [PATCH] for loops modernization

[Alex Rousskov]

On 03/15/2017 06:57 AM, Adam Majer wrote:
> these types of changes
> do not add anything. They actually make the code more difficult to maintain.

I disagree.

> For example,
> 
> -if (HttpRequest *r = dynamic_cast(theMsg)) {
> +if (auto *r = dynamic_cast(theMsg)) {
> 
> Nothing is gained here. On the contrary, it is not immediately known to
> the reader what this `auto` type actually is until they look to the
> right. So what are we saving?

While we do have to "look to the right" and that is indeed a drawback,
auto advantages, collectively, outweigh that negative:

* Auto may reduce explicit code duplication. Saying HttpRequest twice is
code duplication. Code duplication is one of the greatest evils we have
to deal with on a daily basis. It causes functionality bugs and
significantly increases code maintenance overheads.

* Auto reduces implicit code duplication: Saying HttpRequest just
because the type returned by the expression on the right is currently
HttpRequest.

* Auto eliminates implicit type conversions. Some of those conversions
have very undesirable effects, including serious performance degradation.

* In most cases, you either do not need to look to the right (because
you do not care about the actual type specifics) or you have to look to
the right for other reasons (because you care about the code details
after "auto" so you have to keep reading anyway).

More on that below, after the nil pointer discussion (including a
reference to an "authoritative" analysis of your readability argument).


> -Adaptation::Message::Message(): header(NULL)
> +Adaptation::Message::Message(): header(nullptr)
> 
> NULL and nullptr are the same thing for these purposes.

They may or may not be (depending on the header declaration). Since NULL
and nullptr may have different types, they are not identical. More at
http://stackoverflow.com/questions/13816385/what-are-the-advantages-of-using-nullptr


> Actually, the
> most likely implementation of NULL macro in C++11 is just
> 
> #define NULL nullptr

According to the comments at the above URL, that would be illegal
because nullptr is not an integral type and NULL has to have an integral
type.


> I have no problem in using either, but changing this just for the same
> of changing it

Yes, please do not misinterpret my comments as a disagreement on that
very important point: Nullptr is better than NULL, but the negatives in
just changing lots of old working (and needed to be supported across
various branches) code to nullptr outweigh the advantages.


> -for (MECI i = master.extensions.begin(); i != master.extensions.end(); 
> ++i) {
> +for (const auto & extension : master.extensions) {

> [...] this makes is even more complicated to know the type you
> are referring to. The compiler knows, but the developer? I have no idea.
> master.extentions can be anything with iterator.

That is usually a good thing: You do _not_ want to know the exact type
in most cases.


> If you really want to fix this, avoid `auto` in code people actually
> want to maintain. The only place it is actually useful is in heavily
> templated code and that is the main reason it was introduced. It was not
> introduced so you can avoid typing the name of POS (plain old
> structures) or types or classes.

I disagree on all counts. Auto is much more nuanced. The web has a lot
more pro- and cons- arguments about auto, including advice from C++
gurus. For example:
https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/

We should not repeat those debates here IMO. What would be the point? We
are unlikely to come up with better or new arguments... For example, the
above page already discusses your very own readability argument at length!

In my personal experience, going against Sutter and Mayers advice
usually ends up in long-term regrets. They are sometimes wrong, but it
is usually much easier to fix their wrongs than mine. (Just like with
Squid or any other development, it is often about making good mistakes
versus the bad ones.)


Cheers,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Fwd: [PATCH] for loops modernization

[Adam Majer]

On 03/15/2017 03:17 PM, Amos Jeffries wrote:
> Theoretically range-for loops should allow multi-threaded CPU to run
> those loops a bit faster. If that can be demonstrated using a tool like
> polygraph you have a good argument for a patch containing that change to
> go in as a pure performance change.

No. That would break many many things. There are special directives that
allow this to happen with things like OpenMPI compilers, but that's not
what we are talking about here.

And theoretically, if you blindly allow compilers to optimize loops like
that, you are just as likely to introduce hardware stalls that will
result in slower execution of the overall loop. The only way to look at
these,

for (TYPE _i : _c )

is syntactic sugar.


Best regards,
- Adam

PS. And if you are talking about vertorization of these loops, that
already happens with regular loops. See,

https://gcc.gnu.org/projects/tree-ssa/vectorization.html

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Fwd: [PATCH] for loops modernization

[Alex Rousskov]

On 03/15/2017 08:17 AM, Amos Jeffries wrote:

> That said, there are points in the lifecycle which are more friendly to
> bg changes. The next one I expect these C++11 changes can even have a
> chance is after 3.5 ceases to be a supported version.

I agree, especially if "supported" includes "we still have to do a lot
of backporting even if the Squid Project itself no longer does that".


> Theoretically range-for loops should allow multi-threaded CPU to run
> those loops a bit faster. If that can be demonstrated using a tool like
> polygraph you have a good argument for a patch containing that change to
> go in as a pure performance change.

I may agree with the second/non-theoretical part: If changing a loop
makes Squid noticeably faster, then it is a valid reason for the change.
IIRC, some of the changes belong to the non-critical performance paths,
but we might be willing to overlook that if critical path performance is
improved.

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Fwd: [PATCH] for loops modernization

[Alex Rousskov]

On 03/15/2017 04:43 AM, khaled belhout wrote:

> I fixed the naming of the current iteration object, 

Thank you. FWIW, it is not necessary to add _ptr and similar suffixes to
object names when the fact that it is a pointer (or a reference, or a
copy) is unimportant.


> limited the changes to "src/adaptation" sub tree

Why?!


> and replaced NULL macro by nullptr keyword.

You should not have:

* Due to the same porting overhead concerns that I was trying to explain
earlier, we only replace NULLs when the code needs to be changed for
other reasons.

* Mixing benign NULL and potentially deadly (for performance or
const-correctness) auto changes complicates review.


BTW, I am not sure, but I believe the updated patch still seems to be
missing "const" in at least one "auto" conversion case. I did not check
all of them because of the NULL replacement noise.


In summary: I still do not think these changes are desirable (even in
their polished variant), but if others overrule me on that, my comments
about the quality of the current changes still apply.


Thank you,

Alex.


> 2017-03-13 14:45 GMT+01:00 Alex Rousskov :
>> On 03/12/2017 08:42 PM, khaled belhout wrote:
>>> I used clang-tidy tool to modernize all these loops.
>>> http://clang.llvm.org/extra/clang-tidy/
>>
>> It looks like that tool is not ready for fully automated use. If you
>> want to fix its results, please use "const auto" where possible and
>> avoid using "i" for naming the current iteration object.
>>
>>
>>> we can take the advantage of the tool by selecting the changes that
>>> make code more readable understandable and maintainable.
>>
>> Please do not misinterpret my earlier comments as an argument against
>> range loops. Range loops are good and new code should use them!
>>
>> However, I doubt the advantages of changing those old loops outweigh
>> cross-branch development costs right now. Others may disagree, and, if
>> they do, I would not object to a polished patch being committed.
>>
>>
>> Thank you,
>>
>> Alex.
>>
>>
>>> 2017-03-12 16:31 GMT+01:00 Alex Rousskov:
 On 03/12/2017 07:45 AM, khaled belhout wrote:

> this patch modernize for loops using c++11 Range-based for loop

 Please use "const auto" where possible and avoid using "i" for naming
 the current iteration object.

 I am curious why did you decide to change all these loops? How did you
 select the loops to change? Normally, we avoid wide-spread polishing
 touches to minimize the price developers working with older code have to
 pay when porting back various bug fixes and features. I am trying to
 decide whether the advantages of changing these loops outweigh those
 costs in this case.
>>

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Fwd: [PATCH] for loops modernization

[Amos Jeffries]

On 15/03/2017 11:43 p.m., khaled belhout wrote:
> hello Alex
> I fixed the naming of the current iteration object, limited the
> changes to "src/adaptation" sub tree and replaced NULL macro by
> nullptr keyword.
> 

Hi Khaled,

Thanks for the proposals and especially for identifying a tool to detect
the changes automatically.

I would *love* to have all this change away in the past, but I am
(reluctantly) in agreement with Alex regarding these patches.


For several reasons (in no particular order):

2) We have had a lot of discussions about large cleanup like this. While
I am personally in favour of them, there are some very good reasons -
specifically around backporting as Alex metioned.

The current compromise we have in the team is to include this type of
cleanup in all patches - code which gets changed for other reasons gets
polished as well. And in a somewhat slow drip of maintenance changes
done for the C++ conversion which is ongoing since 3.0.

That said, there are points in the lifecycle which are more friendly to
bg changes. The next one I expect these C++11 changes can even have a
chance is after 3.5 ceases to be a supported version. Until that
milestone is reached we have a lot of people relying on 3.5 backports
being reliable and older compilers building the code. Yes that does
cramp the C++11 conversion a bit.


1) Size versus content of the patches.

In a large codebase like Squid there are a lot of hidden dependencies
and logic nobody has read for decades.

In these conditions it is better to isolate each specific polishing task
in its own patch. That way the big changes have a singular, explicit and
clear effect on the code behaviour which can be described in the merge
comment. The audit for large patches is already mind-numbing and not
having to check multiple effects at a time helps a LOT.


3) Not matching our actual needs.

We already have a pretty good idea of what we want the cleanup up code
to look like. It is just the above situations get in the way. The
changes this tool is doing do not go far enough to be what we consider
clean code. There are extra actions which appear to still be manual and
necessary to get that end product.

For example;  running sed -e 's/NULL/nullptr/g' is easy, and the reverse
can be done on 3.5 patches. If that were what we wanted the v4+ code to
look like it would have happened already. We want to do better.

For code to be accepted into Squid nowdays NULL is sometimes replaced
with nullptr, but usually in boolean conditions involving a pointer (or
Pointer object) the equivalence should be dropped entirely.
 if (x == NULL) becomes if (!x), etc.

We have a history of getting constructor initialization lists overly
verbose or incomplete, sometimes both at once. So with C++11 I am also
working towards default initialization for classes which adds another
complication for NULL removal.  That cleanup change means moving the
location of the nullptr (and other trivial types) to the .h definition
and then deciding whether the constructor can be dropped entirey, or
remains as inline (or not) and/or with much shortened initializer list.


* There are some things like HERE macro which are obsolete and should be
simply erased.

Although again with HERE a manual analysis of each code block is needed
to make sure they still make sense. Most can be dropped but certain
messages need changing to use MYNAME instead. And many debugs() texts
predate even the HERE macro itself and need string edits to remove
explicit nameing of a function (which might not be the function/method's
current name! yuck).

... and some other projects cleaning up the code style as much as we can
and removing outdated C object types.


So we have the current policy of when code is touched it gets polished
up according to all the waiting 'cleanup' requirements.

Plus an additional stream of small targeted cleanup patches is going in
all the time in a way that can be audited relatively quickly (if
necessary) and issues identified quickly if there are any.
 You will mostly see these by me at present and mentioning "Cleanup:" or
C++11 in the patch descriptions.



Getting back to your patch submission specifically:

Theoretically range-for loops should allow multi-threaded CPU to run
those loops a bit faster. If that can be demonstrated using a tool like
polygraph you have a good argument for a patch containing that change to
go in as a pure performance change.

Without that evidence or if testing shows no speed gain because compiler
optimization is good - then it is just a very large polish patch and we
have to treat it as such with a "sorry, not now".


Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Fwd: [PATCH] for loops modernization

[Adam Majer]

On 03/15/2017 11:43 AM, khaled belhout wrote:
> hello Alex
> I fixed the naming of the current iteration object, limited the
> changes to "src/adaptation" sub tree and replaced NULL macro by
> nullptr keyword.

I don't want to be too critical about this, but these types of changes
do not add anything. They actually make the code more difficult to maintain.

For example,

-if (HttpRequest *r = dynamic_cast(theMsg)) {
+if (auto *r = dynamic_cast(theMsg)) {

Nothing is gained here. On the contrary, it is not immediately known to
the reader what this `auto` type actually is until they look to the
right. So what are we saving?


-Adaptation::Message::Message(): header(NULL)
+Adaptation::Message::Message(): header(nullptr)

NULL and nullptr are the same thing for these purposes. Actually, the
most likely implementation of NULL macro in C++11 is just

#define NULL nullptr

I have no problem in using either, but changing this just for the same
of changing it

Another example,

 typedef Master::Extensions::const_iterator MECI;
-for (MECI i = master.extensions.begin(); i !=
master.extensions.end(); ++i) {
-if (name == i->first)
-return Area(i->second.data(), i->second.size());
+for (const auto & extension : master.extensions) {
+if (name == extension.first)
+return Area(extension.second.data(), extension.second.size());
 }

In this example, you've not even removed MECI typedef. But basically the
point here is this makes is even more complicated to know the type you
are referring to. The compiler knows, but the developer? I have no idea.
master.extentions can be anything with iterator.

If you really want to fix this, avoid `auto` in code people actually
want to maintain. The only place it is actually useful is in heavily
templated code and that is the main reason it was introduced. It was not
introduced so you can avoid typing the name of POS (plain old
structures) or types or classes.

This is where you *may* want to use auto, like

std::vector>> my_awesome_vector;
for (const auto &p : my_awesome_vector) {

}

But if you have,

std::vector some_vector;
for (const Something &thing : some_vector) {
   ...
}

because using

for (const auto &thing : some_vector) {
   ...
}

doesn't really save you anything, it just makes it worse.

These are just my personal opinions. Others may disagree.

- Adam

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Fwd: [PATCH] for loops modernization

[khaled belhout]

hello Alex
I fixed the naming of the current iteration object, limited the
changes to "src/adaptation" sub tree and replaced NULL macro by
nullptr keyword.



2017-03-13 14:45 GMT+01:00 Alex Rousskov :
> On 03/12/2017 08:42 PM, khaled belhout wrote:
>> I used clang-tidy tool to modernize all these loops.
>> http://clang.llvm.org/extra/clang-tidy/
>
> It looks like that tool is not ready for fully automated use. If you
> want to fix its results, please use "const auto" where possible and
> avoid using "i" for naming the current iteration object.
>
>
>> we can take the advantage of the tool by selecting the changes that
>> make code more readable understandable and maintainable.
>
> Please do not misinterpret my earlier comments as an argument against
> range loops. Range loops are good and new code should use them!
>
> However, I doubt the advantages of changing those old loops outweigh
> cross-branch development costs right now. Others may disagree, and, if
> they do, I would not object to a polished patch being committed.
>
>
> Thank you,
>
> Alex.
>
>
>> 2017-03-12 16:31 GMT+01:00 Alex Rousskov:
>>> On 03/12/2017 07:45 AM, khaled belhout wrote:
>>>
 this patch modernize for loops using c++11 Range-based for loop
>>>
>>> Please use "const auto" where possible and avoid using "i" for naming
>>> the current iteration object.
>>>
>>> I am curious why did you decide to change all these loops? How did you
>>> select the loops to change? Normally, we avoid wide-spread polishing
>>> touches to minimize the price developers working with older code have to
>>> pay when porting back various bug fixes and features. I am trying to
>>> decide whether the advantages of changing these loops outweigh those
>>> costs in this case.
>
diff --git a/src/adaptation/AccessCheck.cc b/src/adaptation/AccessCheck.cc
index 6ce6d91..ddf88f8 100644
--- a/src/adaptation/AccessCheck.cc
+++ b/src/adaptation/AccessCheck.cc
@@ -47,11 +47,11 @@ Adaptation::AccessCheck::AccessCheck(const ServiceFilter &aFilter,
  Adaptation::Initiator *initiator):
 AsyncJob("AccessCheck"), filter(aFilter),
 theInitiator(initiator),
-acl_checklist(NULL)
+acl_checklist(nullptr)
 {
 #if ICAP_CLIENT
 Adaptation::Icap::History::Pointer h = filter.request->icapHistory();
-if (h != NULL)
+if (h != nullptr)
 h->start("ACL");
 #endif
 
@@ -63,7 +63,7 @@ Adaptation::AccessCheck::~AccessCheck()
 {
 #if ICAP_CLIENT
 Adaptation::Icap::History::Pointer h = filter.request->icapHistory();
-if (h != NULL)
+if (h != nullptr)
 h->stop("ACL");
 #endif
 }
@@ -105,12 +105,10 @@ Adaptation::AccessCheck::check()
 {
 debugs(93, 4, HERE << "start checking");
 
-typedef AccessRules::iterator ARI;
-for (ARI i = AllRules().begin(); i != AllRules().end(); ++i) {
-AccessRule *r = *i;
-if (isCandidate(*r)) {
-debugs(93, 5, HERE << "check: rule '" << r->id << "' is a candidate");
-candidates.push_back(r->id);
+for (auto rule_ptr : AllRules()) {
+if (isCandidate(*rule_ptr)) {
+debugs(93, 5, HERE << "check: rule '" << rule_ptr->id << "' is a candidate");
+candidates.push_back(rule_ptr->id);
 }
 }
 
@@ -143,7 +141,7 @@ Adaptation::AccessCheck::checkCandidates()
 }
 
 debugs(93, 4, HERE << "NO candidates left");
-callBack(NULL);
+callBack(nullptr);
 Must(done());
 }
 
@@ -151,7 +149,7 @@ void
 Adaptation::AccessCheck::AccessCheckCallbackWrapper(allow_t answer, void *data)
 {
 debugs(93, 8, HERE << "callback answer=" << answer);
-AccessCheck *ac = (AccessCheck*)data;
+auto *ac = (AccessCheck*)data;
 
 /** \todo AYJ 2008-06-12: If answer == ACCESS_AUTH_REQUIRED
  * we should be kicking off an authentication before continuing
@@ -176,7 +174,7 @@ Adaptation::AccessCheck::noteAnswer(allow_t answer)
 
 if (answer == ACCESS_ALLOWED) { // the rule matched
 ServiceGroupPointer g = topGroup();
-if (g != NULL) { // the corresponding group found
+if (g != nullptr) { // the corresponding group found
 callBack(g);
 Must(done());
 return;
diff --git a/src/adaptation/AccessRule.cc b/src/adaptation/AccessRule.cc
index 2378d57..2baa59a 100644
--- a/src/adaptation/AccessRule.cc
+++ b/src/adaptation/AccessRule.cc
@@ -17,7 +17,7 @@
 
 int Adaptation::AccessRule::LastId = 0;
 
-Adaptation::AccessRule::AccessRule(const String &aGroupId): id(++LastId), groupId(aGroupId), acl(NULL)
+Adaptation::AccessRule::AccessRule(const String &aGroupId): id(++LastId), groupId(aGroupId), acl(nullptr)
 {
 }
 
@@ -38,7 +38,7 @@ Adaptation::AccessRule::finalize()
 if (!group()) { // no explicit group
 debugs(93,7, HERE << "no service group: " << groupId);
 // try to add a one-service group
-if (FindService(groupId) != NULL) {
+if (FindService(groupId) != nullptr) {
 ServiceGroupPointer g = new 

Re: [squid-dev] To make squid works in snap world.

[Gary Wang]

Regarding the confinement of usage of shared memory in snap world,
   Please take a look at the bug
   https://bugs.launchpad.net/snappy/+bug/1653955
   And Jamie's reply can be fuond in snapcraft maillist
   https://www.mail-archive.com/snapcraft@lists.snapcraft.io/
msg02465.html

"sem_open() is https://bugs.launchpad.net/snappy/+bug/1653955 and
 snapd 2.21

added support to allow /{dev,run}/shm/sem.snap.@{SNAP_NAME}.*. This is

sufficient to make use of sem_open() possible."

On Wed, Mar 15, 2017 at 12:48 PM, Alex Rousskov <
rouss...@measurement-factory.com> wrote:

> On 03/14/2017 09:52 PM, Gary Wang wrote:
> > On Tue, Mar 14, 2017 at 11:49 PM, Alex Rousskov wrote:
> >> On 03/14/2017 08:44 AM, Gary Wang wrote:
> >> > About the DEFAULT_STATEDIR,
> >> > ...
> >> > DEFS += -DDEFAULT_STATEDIR=\"$(localstatedir)/run/squid\"
> >> > ...
> >> > According to the above share memory namespace in snap world,
> it
> >> > couldn't help on this either.
>
> >> Why not make DEFAULT_STATEDIR configurable instead?
>
> >A: Actually, DEFAULT_IPC_PREFIX only changes the shared memory file
> name
> >not the file path. So the usage of these two options are
> different.
>
> You are describing your implementation. I am asking why not support the
> same functionality using a _different_ general implementation that
> avoids adding a yet another option that controls IPC-related file names.
> You added an option that changes the "middle" part of the full file
> name. Why not make the existing constant that controls the "prefix" part
> of the full file name configurable instead?
>
> I understand that the two approaches result in [slightly] different
> code, but if a single option can accomplish the desired outcome and
> supports more use cases, then using that single option may be a better
> approach.
>
>
> > DEFAULT_STATEDIR only helps the case that the segment name is path.
>
> Can the updated/renamed DEFAULT_STATEDIR be interpreted as
>
>   / 
>
> where the first component is used if and only when nameIsPath is true
> and the second component is optional? AFAICT, such an implementation
> would cover the already supported use cases and your new use case.
>
> For snap environments, the option could be set to something like:
>
>   $(localstatedir)/run/squid/sem.snap.squid.
>
>   A: I understood your point that a single option can keep clean code base.
  But interpreting  DEFAULT_STATEDIR  as  / 
  is more like a trick. Wihout snap-related knowledge, normal developer
will
  treat the the option as an absolute path instead of combination
  of /

  I can make a change to fix it based on your comment.
  But please double-check if it's the right implementation.

  Meanwhile, according to shared memory file namespace
   /dev/shm/sem.snap.@{SNAP_NAME}.*
  If we use the option in the following way
  $(localstatedir)/run/squid/sem.snap.squid,
  which means SNAP_NAME would be "squid",
 I have a quick look at ubuntu store and found that "squid" snap name
is registered by
 other developers. To avoid name disputes, there're two options
 1. change
  $(localstatedir)/run/squid/sem.snap.squid.
 to
  $(localstatedir)/run/squid/sem.snap.squid-snap.
 2. make STATEDIR configurable so developer can customize it.
 I perfer the latter one.

>
> > Another concern is that even though we make DEFAULT_STATEDIR
> > configurable instead of introducing new option, STATEDIR, IPCDIR is not
> > suitable as end-user might specify the dir path for these options
> however the
> > purpose of DEFAULT_IPC_PREFIX is only add prefix to shared memory file
> name
>
> I am not sure I understand the above concern, but perhaps my suggestion
> to ignore the path component (as needed) would address it?
>
>
> >> AFAICT, DEFAULT_STATEDIR is also used for IPC communications in
> >> src/ipc/Port.cc. Your patch does not change that code. This means
> that
> >>
> >> * either you forgot to change src/ipc/Port.cc to honor the new
> >> DEFAULT_IPC_PREFIX
>
> > A: I will change src/ipc/Port.cc in the next commit once we reach an
> > agreement on the usage of compiler option.
>
> For Squid to work in a snap world, will you need the same
> sem.snap.@{SNAP_NAME}. prefix for Unix Domain Sockets as for shared
> memory segments?
>
> * If yes, then using a single option for both ipc/mem/Segment.cc and
> ipc/Port.cc may work.
>
> * Otherwise, we would have to add two different options, one for shared
> memory segment names and one for UDS names.
>
>A: No, I don't need the same prefix for UDS names. As you said, we might
   need two different options.

>
> Cheers,
>
> Alex.
>
>
> > * or src/ipc/Port.cc cannot use DEFAULT_IPC_PREFIX for some reason
> (and,
> > hence, DEFAULT_IPC_PREFIX is the wrong name because the parameter
> does
> > 

Re: [squid-dev] To make squid works in snap world.

[Eliezer Croitoru]

+1

How can I reproduce he error?
Is there a bug report open for this issue?

Thanks,
Eliezer


Eliezer Croitoru
Linux System Administrator
Mobile: +972-5-28704261
Email: elie...@ngtech.co.il


-Original Message-
From: squid-dev [mailto:squid-dev-boun...@lists.squid-cache.org] On Behalf Of 
Amos Jeffries
Sent: Wednesday, March 15, 2017 6:43 AM
To: squid-dev@lists.squid-cache.org
Subject: Re: [squid-dev] To make squid works in snap world.

On 15/03/2017 3:44 a.m., Gary Wang wrote:
> Hi guys
> I'm sorry that I'm here so late. :(
> Generally, regarding the purpose of this MP.
> 
> https://code.launchpad.net/~gary-wzl77/squid/ipc_prefix/+merge/318714
> 
> I'd like to make squid snap works as a confined 
> snap in snap world. 
> So that we can ship this snap in ubuntu-core.
> The reason why I need to add compile option to enable to customize 
> IPC prefix at compiling time is that in order to use shared memory in 
> an app which released as a snap package the only allowed file path 
> will be like this  (in 
> the following
> namespace)
>  /dev/shm/sem.snap.@{SNAP_NAME}.*
> 
> Hence in our case, the shared memory file path should be
> /dev/shm/sem.snap.squid-snap.{random-string}
> Otherwise, you will get the following error when running the squid 
> in snap world
> http://paste.ubuntu.com/24175840/
> 

Having looked at this a lot more now I think the patch is based on an incorrect 
assumption.

You see Squid complaining of /dev/shm Permissions error. Other people getting 
that error in snap world were using semaphores and fixed it by using snap 
/dev/shm/sem.* names. So you fixed the /dev/shm naming to match snap semaphore 
naming.

... but Squid does *not* use semaphores.

Simply making Squid pretend to be doing semaphores to bypass the security is 
not the right way forward.

The real question is why the permissions error is occuring?

What in snap world is refusing permission?

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev