Re: /bzr/squid3/trunk/ r12194: Small optimization in CommOpener statistic accounting.
tis 2012-07-03 klockan 07:52 +0200 skrev Kinkie: ++(conn_-getPeer()-stats.conn_open); IMHO we should consistently use bracketing as above to clarify in situations like this where there is any complex location syntax. It is the latter, but I had the some doubt so I double-checked. I agree however that this may be not as readable as I'd like. Ok for the bracketing, I'll review the cases that snuck in and correct them. Usually the other ++ works better in readability conn_-getPeer()-stats.conn_open++; or even conn_-getPeer()-stats.conn_open += 1; Regards Henrik
Re: /bzr/squid3/trunk/ r12194: Small optimization in CommOpener statistic accounting.
Usually the other ++ works better in readability conn_-getPeer()-stats.conn_open++; or even conn_-getPeer()-stats.conn_open += 1; With GCC this code: 15 int a=0; 16 a++; 17 ++a; 18 a+=1; gets assembled as: 16 a++; = 0x0804873c main(int, char**)+168: 83 44 24 18 01 addl $0x1,0x18(%esp) (gdb) 17 ++a; = 0x08048741 main(int, char**)+173: 83 44 24 18 01 addl $0x1,0x18(%esp) (gdb) 18 a+=1; = 0x08048746 main(int, char**)+178: 83 44 24 18 01 addl $0x1,0x18(%esp) -- /kinkie
Re: /bzr/squid3/trunk/ r12194: Small optimization in CommOpener statistic accounting.
On 07/02/2012 04:34 PM, Amos Jeffries wrote: On 03.07.2012 03:30, Francesco Chemolli wrote: -if (conn_-getPeer()) -conn_-getPeer()-stats.conn_open++; +if (peer *peer=(conn_-getPeer())) +++peer-stats.conn_open; lookupLocalAddress(); Two points: 1) assignment in the if() needs to be double-bracketed around the whole = operator expression, not just the RHS: if ((peer *peer=conn_-getPeer())) No, please do not do that! This is not a regular assignment, it is a variable declaration with initialization. It does not need more brackets. In fact, I would not be surprised if some compilers will fail if those extra brackets are included. The problem with that expression is that the variable name shadows the type (it is usually not a bug due to C++ name resolution rules but bad style). It also uses too many brackets on the RHS. I know that some like extra brackets (and then skimp on spaces, resulting in a really hard-to-read stream of characters!) so I am not going to fight about this aspect. The best syntax in this particular context is if (peer *p = conn_-getPeer()) HTH, Alex.
Re: /bzr/squid3/trunk/ r12194: Small optimization in CommOpener statistic accounting.
On 03.07.2012 21:27, Kinkie wrote: Usually the other ++ works better in readability conn_-getPeer()-stats.conn_open++; or even conn_-getPeer()-stats.conn_open += 1; With GCC this code: 15 int a=0; 16 a++; 17 ++a; 18 a+=1; gets assembled as: 16 a++; = 0x0804873c main(int, char**)+168: 83 44 24 18 01 addl $0x1,0x18(%esp) (gdb) 17 ++a; = 0x08048741 main(int, char**)+173: 83 44 24 18 01 addl $0x1,0x18(%esp) (gdb) 18 a+=1; = 0x08048746 main(int, char**)+178: 83 44 24 18 01 addl $0x1,0x18(%esp) That matches what the recent blog post benchmarks are saying. Con: * Modern compilers are able usually to optimize all forms automatically to one instruction. * when they optimize to 2 instructions, those are usually run in parallel by the CPU optimizations anyway. * +=1 is sometimes (when?) benchmarked as faster than both by about 2x the difference between the ++ themselves. * C++11 compilers contain a lot of optimizations (like auto-merging these) which dwarf these manual benefits. Pros: (apparently) * -O0 builds apparently have some difference, (but then does anyone care with -O0?) * its easier to code a pre-increment operator on custom classes than a post. (do we care?) * pre-increment on compound objects does not undergo auto-magical conversion to operator+=() by default. Apparently post-increment does. * there is apparently still a difference on some less common CPU architectures. PowerPC was brought up as an example. No mention of ARM, so I'd be interested to know if ARM has any difference. IMO, * This last two pros appear to make it still worthwhile for portable software like Squid. For now. * this is the type of minor optimization that we should accept as it comes, but not waste time arguing over (or obfuscating the code to achieve!). It's too small an optimization to be worth the trouble. * my point about ++conn_-getPeer()-stats.conn_open is solely down to readability. Which is trivially resolved by ++(x), with brackets around the symbol being incremented. Using bracketing just for clarity covers all sorts of borderline expressions which appear obfuscated at first glance. Amos
Re: /bzr/squid3/trunk/ r12194: Small optimization in CommOpener statistic accounting.
On 03.07.2012 03:30, Francesco Chemolli wrote: -if (conn_-getPeer()) -conn_-getPeer()-stats.conn_open++; +if (peer *peer=(conn_-getPeer())) +++peer-stats.conn_open; lookupLocalAddress(); Two points: 1) assignment in the if() needs to be double-bracketed around the whole = operator expression, not just the RHS: if ((peer *peer=conn_-getPeer())) 2) This is the type of pre-increment operator use which I am a bit uncomfortable with. It is hard to tell when skimming the code at speed whether that should be read as: (++peer)-stats.conn_open; or ++(conn_-getPeer()-stats.conn_open); IMHO we should consistently use bracketing as above to clarify in situations like this where there is any complex location syntax. Amos
Re: /bzr/squid3/trunk/ r12194: Small optimization in CommOpener statistic accounting.
On Tue, Jul 3, 2012 at 12:34 AM, Amos Jeffries squ...@treenet.co.nz wrote: On 03.07.2012 03:30, Francesco Chemolli wrote: -if (conn_-getPeer()) -conn_-getPeer()-stats.conn_open++; +if (peer *peer=(conn_-getPeer())) +++peer-stats.conn_open; lookupLocalAddress(); Two points: 1) assignment in the if() needs to be double-bracketed around the whole = operator expression, not just the RHS: if ((peer *peer=conn_-getPeer())) gcc accepts it, but sure. 2) This is the type of pre-increment operator use which I am a bit uncomfortable with. It is hard to tell when skimming the code at speed whether that should be read as: (++peer)-stats.conn_open; or ++(conn_-getPeer()-stats.conn_open); IMHO we should consistently use bracketing as above to clarify in situations like this where there is any complex location syntax. It is the latter, but I had the some doubt so I double-checked. I agree however that this may be not as readable as I'd like. Ok for the bracketing, I'll review the cases that snuck in and correct them. -- /kinkie
