Re: [PATCH] Solaris /dev/poll support for Squid 3 (how can I contribute)
On 10/19/10 18:40, Alex Rousskov wrote: On 10/13/2010 04:21 AM, Peter Payne wrote: BENCHMARKS Firstly, I wanted to address the benchmark questions as it made me curious as to whether there really was an advantage in using /dev/poll. I used the Apache Bench tool (that comes with HTTPD) to do my benchmarks. I compiled a 32-bit Solaris 5.10 x86 of Squid version 3.1.8, and another Squid version 3.1.8 with my patches. I shall call them "unpatched Squid" and "patched Squid" respectively. Note that "unpatched Squid" was using ordinary poll() and "patched Squid" was using /dev/poll. Where I used Apache bench with 1 simultaneous connection or even 8,000 simultaneous connections there was little difference between "unpatched Squid" and "patched Squid". When all connections are busy there is no performance gain. However the test that proved the most striking difference was pre-establishing 8,000 TCP socket connections to Squid using a basic Perl script. Then running Apache Bench with 1 connection making 100,000 keep-alive'd requests. unpatched Squid (using poll): 2min23sec CPU user-time 171sec to process 10 Apache Bench queries patched Squid (using /dev/poll): 0min22sec CPU user-time 25sec to process 10 Apache Bench queries Clearly /dev/poll has a significant performance gain where there are many idle TCP socket connections. On 10/13/2010 04:25 PM, Amos Jeffries wrote: > Oooh... Speed. Not really, at least not if you define "performance" or "speed" as "ability to sustain an X requests/second load". The above test is a classic best-effort test that, by design, cannot measure proxy's ability to handle load because its offered request rate depends on measured response time. During such a test, Squid drives the benchmark rather than the benchmark driving Squid. There are many real-world examples where a faster product (for any practical definition of "faster") would show drastically worse results during such a test. What the test does tell you is that an idle Squid became faster at processing a single transaction after the patch. "Significant performance gain" and similar conclusions are premature after a best-effort test like this one. Please do _not_ interpret my comments as negative towards the patch itself. I just want to minimize the chance that others will start running similar tests and jumping to the wrong conclusions regarding their own optimization ideas. Thank you, Alex. Alex, I completely agree. During testing there was little or no performance gain when processing n busy connections (all busy). The only performance benefit arose out of servicing few busy connections when many were idle. As for the realism or effectiveness in the real world it may be negligible. However there are situations when a connection is idle: when waiting for a proxy to fetch a page from somewhere else, when holding a connection open using keep-alive. At any rate, it was a desired feature on the Squid 3 roadmap and was already provided for Squid 2, and given that epoll and kqueue support exists, it is added for completeness (with a LOT more comments in the source too, initially I found reading the older code harder going). Thanks, Peter
Re: [PATCH] Solaris /dev/poll support for Squid 3 (how can I contribute)
On 10/13/2010 04:21 AM, Peter Payne wrote: BENCHMARKS Firstly, I wanted to address the benchmark questions as it made me curious as to whether there really was an advantage in using /dev/poll. I used the Apache Bench tool (that comes with HTTPD) to do my benchmarks. I compiled a 32-bit Solaris 5.10 x86 of Squid version 3.1.8, and another Squid version 3.1.8 with my patches. I shall call them "unpatched Squid" and "patched Squid" respectively. Note that "unpatched Squid" was using ordinary poll() and "patched Squid" was using /dev/poll. Where I used Apache bench with 1 simultaneous connection or even 8,000 simultaneous connections there was little difference between "unpatched Squid" and "patched Squid". When all connections are busy there is no performance gain. However the test that proved the most striking difference was pre-establishing 8,000 TCP socket connections to Squid using a basic Perl script. Then running Apache Bench with 1 connection making 100,000 keep-alive'd requests. unpatched Squid (using poll): 2min23sec CPU user-time 171sec to process 10 Apache Bench queries patched Squid (using /dev/poll): 0min22sec CPU user-time 25sec to process 10 Apache Bench queries Clearly /dev/poll has a significant performance gain where there are many idle TCP socket connections. On 10/13/2010 04:25 PM, Amos Jeffries wrote: > Oooh... Speed. Not really, at least not if you define "performance" or "speed" as "ability to sustain an X requests/second load". The above test is a classic best-effort test that, by design, cannot measure proxy's ability to handle load because its offered request rate depends on measured response time. During such a test, Squid drives the benchmark rather than the benchmark driving Squid. There are many real-world examples where a faster product (for any practical definition of "faster") would show drastically worse results during such a test. What the test does tell you is that an idle Squid became faster at processing a single transaction after the patch. "Significant performance gain" and similar conclusions are premature after a best-effort test like this one. Please do _not_ interpret my comments as negative towards the patch itself. I just want to minimize the chance that others will start running similar tests and jumping to the wrong conclusions regarding their own optimization ideas. Thank you, Alex.
Re: [PATCH] Solaris /dev/poll support for Squid 3 (how can I contribute)
On 14/10/10 13:58, Henrik Nordström wrote: ons 2010-10-13 klockan 22:25 + skrev Amos Jeffries: +1 from me with merge tweaks. Unless anyone has objections I will commit with tweaks at the next opportunity. No objection from me. But have not reviewed the changes outside comm_devpoll.cc. Regards Henrik The rest being minimal basic plumbing. I'm taking this as a second vote. :) Applied to Squid-3 as r10946. Thank you Peter. Amos -- Please be using Current Stable Squid 2.7.STABLE9 or 3.1.8 Beta testers wanted for 3.2.0.2
Re: [PATCH] Solaris /dev/poll support for Squid 3 (how can I contribute)
ons 2010-10-13 klockan 22:25 + skrev Amos Jeffries: > > +1 from me with merge tweaks. > > Unless anyone has objections I will commit with tweaks at the next > opportunity. No objection from me. But have not reviewed the changes outside comm_devpoll.cc. Regards Henrik
Re: [PATCH] Solaris /dev/poll support for Squid 3 (how can I contribute)
On Wed, 13 Oct 2010 15:40:22 +0100, Peter Payne wrote: > Hello Amos, > > apologies to the dev list for what must appear to be spamming. > Completely on topic. No apologies needed. > Please use the attached version of the file. It has a minor performance > tweak (only set events that have changed if not clearing any events). > > Compiled using CC=/tool/sunstudio12/bin/cc and tested again using Apache > Bench and some manual queries through a web browser. > > Peter > Will do. Thanks. > On 10/13/10 11:21, Peter Payne wrote: >> Hello Amos, >> >> in response to your comments. >> >> >> BENCHMARKS >> >> Firstly, I wanted to address the benchmark questions as it made me >> curious as to whether there really was an advantage in using >> /dev/poll. I used the Apache Bench tool (that comes with HTTPD) to do >> my benchmarks. >> >> I compiled a 32-bit Solaris 5.10 x86 of Squid version 3.1.8, and >> another Squid version 3.1.8 with my patches. I shall call them >> "unpatched Squid" and "patched Squid" respectively. Note that >> "unpatched Squid" was using ordinary poll() and "patched Squid" was >> using /dev/poll. >> >> Where I used Apache bench with 1 simultaneous connection or even 8,000 >> simultaneous connections there was little difference between >> "unpatched Squid" and "patched Squid". When all connections are busy >> there is no performance gain. >> >> However the test that proved the most striking difference was >> pre-establishing 8,000 TCP socket connections to Squid using a basic >> Perl script. Then running Apache Bench with 1 connection making >> 100,000 keep-alive'd requests. >> >> unpatched Squid (using poll): >> 2min23sec CPU user-time >> 171sec to process 10 Apache Bench queries >> >> patched Squid (using /dev/poll): >> 0min22sec CPU user-time >> 25sec to process 10 Apache Bench queries >> Oooh... Speed. >> Clearly /dev/poll has a significant performance gain where there are >> many idle TCP socket connections. >> >> I have now also optimised the comm_devpoll.cc routine to ensure >> unnecessary calls to /dev/poll with the POLLREMOVE flag were removed. >> >> >> CODE AUDIT >> >> Most of the style used was a result of directly copying the styles >> used in the existing comm_devpoll.c from Squid v2.7 and comm_epoll.cc >> from Squid v3.1.8. However where these conflict with the requests made >> most recently I will adopt the recommendations in the e-mail. >> >> 1. used #if USE_DEVPOLL instead of #ifdef >> 2. moved #includes except squid.h inside USE_DEVPOLL wrapper >> 3. moved comment to own line >> 4. removed big XXX..XXX lines (note comm_epoll.cc uses this notation) >> 5. made big doxygen effort!!! Enjoy. >> 6. (did not remove no-op else cases, I think they are clear) >> 7. used HERE macro as requested >> >> >> SQUID-COMPAT.DIFF >> >> /dev/poll doesn't need to be hard coded, that was a mistake. Please >> migrate USE_DEVPOLL conditional to same line as USE_EPOLL. >> K. will do >> >> SQUID-ROOT.DIFF >> >> Pretty much all beyond me, am happy with whatever changes you decide >> to make. I just wanted to add a /dev/poll auto-detect routine and use >> that in preference to poll() if available. >> >> Note that epoll/kqueue is assumed to be unavailable on any Solaris >> system with /dev/poll. You might want to thus prioritise /dev/poll >> support to BELOW that of epoll and kqueue (in case there's any >> confusion). But of course /dev/poll should be by default selected if >> available before defaulting to standard poll(). Okay. Thanks for that. >> >> Kind regards, >> Peter Payne >> +1 from me with merge tweaks. Unless anyone has objections I will commit with tweaks at the next opportunity. Amos
Re: [PATCH] Solaris /dev/poll support for Squid 3 (how can I contribute)
ons 2010-10-13 klockan 15:40 +0100 skrev Peter Payne: > Hello Amos, > > apologies to the dev list for what must appear to be spamming. No apologies needed. We are all for release early an often, and discussing code is what this list is for. Regards Henrik
Re: [PATCH] Solaris /dev/poll support for Squid 3 (how can I contribute)
Hello Amos, apologies to the dev list for what must appear to be spamming. Please use the attached version of the file. It has a minor performance tweak (only set events that have changed if not clearing any events). Compiled using CC=/tool/sunstudio12/bin/cc and tested again using Apache Bench and some manual queries through a web browser. Peter On 10/13/10 11:21, Peter Payne wrote: Hello Amos, in response to your comments. BENCHMARKS Firstly, I wanted to address the benchmark questions as it made me curious as to whether there really was an advantage in using /dev/poll. I used the Apache Bench tool (that comes with HTTPD) to do my benchmarks. I compiled a 32-bit Solaris 5.10 x86 of Squid version 3.1.8, and another Squid version 3.1.8 with my patches. I shall call them "unpatched Squid" and "patched Squid" respectively. Note that "unpatched Squid" was using ordinary poll() and "patched Squid" was using /dev/poll. Where I used Apache bench with 1 simultaneous connection or even 8,000 simultaneous connections there was little difference between "unpatched Squid" and "patched Squid". When all connections are busy there is no performance gain. However the test that proved the most striking difference was pre-establishing 8,000 TCP socket connections to Squid using a basic Perl script. Then running Apache Bench with 1 connection making 100,000 keep-alive'd requests. unpatched Squid (using poll): 2min23sec CPU user-time 171sec to process 10 Apache Bench queries patched Squid (using /dev/poll): 0min22sec CPU user-time 25sec to process 10 Apache Bench queries Clearly /dev/poll has a significant performance gain where there are many idle TCP socket connections. I have now also optimised the comm_devpoll.cc routine to ensure unnecessary calls to /dev/poll with the POLLREMOVE flag were removed. CODE AUDIT Most of the style used was a result of directly copying the styles used in the existing comm_devpoll.c from Squid v2.7 and comm_epoll.cc from Squid v3.1.8. However where these conflict with the requests made most recently I will adopt the recommendations in the e-mail. 1. used #if USE_DEVPOLL instead of #ifdef 2. moved #includes except squid.h inside USE_DEVPOLL wrapper 3. moved comment to own line 4. removed big XXX..XXX lines (note comm_epoll.cc uses this notation) 5. made big doxygen effort!!! Enjoy. 6. (did not remove no-op else cases, I think they are clear) 7. used HERE macro as requested SQUID-COMPAT.DIFF /dev/poll doesn't need to be hard coded, that was a mistake. Please migrate USE_DEVPOLL conditional to same line as USE_EPOLL. SQUID-ROOT.DIFF Pretty much all beyond me, am happy with whatever changes you decide to make. I just wanted to add a /dev/poll auto-detect routine and use that in preference to poll() if available. Note that epoll/kqueue is assumed to be unavailable on any Solaris system with /dev/poll. You might want to thus prioritise /dev/poll support to BELOW that of epoll and kqueue (in case there's any confusion). But of course /dev/poll should be by default selected if available before defaulting to standard poll(). Kind regards, Peter Payne On 10/12/10 00:36, Amos Jeffries wrote: On Mon, 11 Oct 2010 14:58:34 +0100, Peter Payne wrote: Hello Amos, thank you for your response. Please find attached the four files in question. Companies to mention as sponsors are BBC (UK), Siemens IT Solutions and Services (UK). Credit to Peter Payne, Pirosa Limited UK (no e-mail please). Sure. No problem. Squid bug 3057 was raised by another member of Siemens IT Solutions and Services during testing of Squid 3.1.8 on Solaris - that bug was not produced on the 32-bit compile, just the 64-bit compile. It is not thought this is related to the /dev/poll support added here. Okay. On to the audit. This is just a 3.x style and consistency check. I'll take it as tested and working. comm_devpoll.cc: please make the following updates - use "#if USE_DEVPOLL" instead of #ifdef. - move all #includes except squid.h inside the USE_DEVPOLL wrapper. - move the "/* Solaris /dev/poll support */" to its own line or remove completely. - remove the big "..." lines. - use /** for the function documentation. If you know doxygen syntax please markup the text in any other relevant ways as well. - replace all debugs "DEBUG_DEVPOLL ? 0 : 8" with "5" or "7" to match the other comm loops display density. - replace comm_update_fd the #if DEBUG_DEVPOLL with (?:) constructs in the debugs() ie<< (events& POLLIN ? " IN":"") - in commSetSelect please remove the no-op else cases. "// else: we want ... " comments are fine. - comm_select please use HERE macro in debugs() instead of the text name of the function. squid-include.diff all done by autoconf during packaging. This can be dropped. squid-src.diff: - automake changes need to be made in Makefile.am. This can be done on commit. -
Re: [PATCH] Solaris /dev/poll support for Squid 3 (how can I contribute)
Hello Amos, in response to your comments. BENCHMARKS Firstly, I wanted to address the benchmark questions as it made me curious as to whether there really was an advantage in using /dev/poll. I used the Apache Bench tool (that comes with HTTPD) to do my benchmarks. I compiled a 32-bit Solaris 5.10 x86 of Squid version 3.1.8, and another Squid version 3.1.8 with my patches. I shall call them "unpatched Squid" and "patched Squid" respectively. Note that "unpatched Squid" was using ordinary poll() and "patched Squid" was using /dev/poll. Where I used Apache bench with 1 simultaneous connection or even 8,000 simultaneous connections there was little difference between "unpatched Squid" and "patched Squid". When all connections are busy there is no performance gain. However the test that proved the most striking difference was pre-establishing 8,000 TCP socket connections to Squid using a basic Perl script. Then running Apache Bench with 1 connection making 100,000 keep-alive'd requests. unpatched Squid (using poll): 2min23sec CPU user-time 171sec to process 10 Apache Bench queries patched Squid (using /dev/poll): 0min22sec CPU user-time 25sec to process 10 Apache Bench queries Clearly /dev/poll has a significant performance gain where there are many idle TCP socket connections. I have now also optimised the comm_devpoll.cc routine to ensure unnecessary calls to /dev/poll with the POLLREMOVE flag were removed. CODE AUDIT Most of the style used was a result of directly copying the styles used in the existing comm_devpoll.c from Squid v2.7 and comm_epoll.cc from Squid v3.1.8. However where these conflict with the requests made most recently I will adopt the recommendations in the e-mail. 1. used #if USE_DEVPOLL instead of #ifdef 2. moved #includes except squid.h inside USE_DEVPOLL wrapper 3. moved comment to own line 4. removed big XXX..XXX lines (note comm_epoll.cc uses this notation) 5. made big doxygen effort!!! Enjoy. 6. (did not remove no-op else cases, I think they are clear) 7. used HERE macro as requested SQUID-COMPAT.DIFF /dev/poll doesn't need to be hard coded, that was a mistake. Please migrate USE_DEVPOLL conditional to same line as USE_EPOLL. SQUID-ROOT.DIFF Pretty much all beyond me, am happy with whatever changes you decide to make. I just wanted to add a /dev/poll auto-detect routine and use that in preference to poll() if available. Note that epoll/kqueue is assumed to be unavailable on any Solaris system with /dev/poll. You might want to thus prioritise /dev/poll support to BELOW that of epoll and kqueue (in case there's any confusion). But of course /dev/poll should be by default selected if available before defaulting to standard poll(). Kind regards, Peter Payne On 10/12/10 00:36, Amos Jeffries wrote: On Mon, 11 Oct 2010 14:58:34 +0100, Peter Payne wrote: Hello Amos, thank you for your response. Please find attached the four files in question. Companies to mention as sponsors are BBC (UK), Siemens IT Solutions and Services (UK). Credit to Peter Payne, Pirosa Limited UK (no e-mail please). Sure. No problem. Squid bug 3057 was raised by another member of Siemens IT Solutions and Services during testing of Squid 3.1.8 on Solaris - that bug was not produced on the 32-bit compile, just the 64-bit compile. It is not thought this is related to the /dev/poll support added here. Okay. On to the audit. This is just a 3.x style and consistency check. I'll take it as tested and working. comm_devpoll.cc: please make the following updates - use "#if USE_DEVPOLL" instead of #ifdef. - move all #includes except squid.h inside the USE_DEVPOLL wrapper. - move the "/* Solaris /dev/poll support */" to its own line or remove completely. - remove the big "..." lines. - use /** for the function documentation. If you know doxygen syntax please markup the text in any other relevant ways as well. - replace all debugs "DEBUG_DEVPOLL ? 0 : 8" with "5" or "7" to match the other comm loops display density. - replace comm_update_fd the #if DEBUG_DEVPOLL with (?:) constructs in the debugs() ie<< (events& POLLIN ? " IN":"") - in commSetSelect please remove the no-op else cases. "// else: we want ... " comments are fine. - comm_select please use HERE macro in debugs() instead of the text name of the function. squid-include.diff all done by autoconf during packaging. This can be dropped. squid-src.diff: - automake changes need to be made in Makefile.am. This can be done on commit. - unlinkd.cc changes okay. squid-compat.diff: - changes okay. - but can you mention why /dev/poll limit is hard-coded? compat area needs enough details so we can track the when and why for each hack. The aim being to remove them when the problems have been resolved. squid-root.diff: - okay as-is for 3.1. - for 3.2 and later we have simpler configure.in logics with some helper macros. That can be cu
Re: [PATCH] Solaris /dev/poll support for Squid 3 (how can I contribute)
On Mon, 11 Oct 2010 14:58:34 +0100, Peter Payne wrote: > Hello Amos, > > thank you for your response. Please find attached the four files in > question. > > Companies to mention as sponsors are BBC (UK), Siemens IT Solutions and > Services (UK). > > Credit to Peter Payne, Pirosa Limited UK (no e-mail please). Sure. No problem. > > Squid bug 3057 was raised by another member of Siemens IT Solutions and > Services during testing of Squid 3.1.8 on Solaris - that bug was not > produced on the 32-bit compile, just the 64-bit compile. It is not > thought this is related to the /dev/poll support added here. Okay. On to the audit. This is just a 3.x style and consistency check. I'll take it as tested and working. comm_devpoll.cc: please make the following updates - use "#if USE_DEVPOLL" instead of #ifdef. - move all #includes except squid.h inside the USE_DEVPOLL wrapper. - move the "/* Solaris /dev/poll support */" to its own line or remove completely. - remove the big "..." lines. - use /** for the function documentation. If you know doxygen syntax please markup the text in any other relevant ways as well. - replace all debugs "DEBUG_DEVPOLL ? 0 : 8" with "5" or "7" to match the other comm loops display density. - replace comm_update_fd the #if DEBUG_DEVPOLL with (?:) constructs in the debugs() ie << (events & POLLIN ? " IN":"") - in commSetSelect please remove the no-op else cases. "// else: we want ... " comments are fine. - comm_select please use HERE macro in debugs() instead of the text name of the function. squid-include.diff all done by autoconf during packaging. This can be dropped. squid-src.diff: - automake changes need to be made in Makefile.am. This can be done on commit. - unlinkd.cc changes okay. squid-compat.diff: - changes okay. - but can you mention why /dev/poll limit is hard-coded? compat area needs enough details so we can track the when and why for each hack. The aim being to remove them when the problems have been resolved. squid-root.diff: - okay as-is for 3.1. - for 3.2 and later we have simpler configure.in logics with some helper macros. That can be cut-n-pasted on commit. - RUNIFELSE is a bigger problem. It breaks cross-compiling and I notice its not present in 2.7. Is the configure-time run test marked "Verify that /dev/poll really works" actually needed? What will happen to builds made on non-Solaris with forced devpoll and the kernel headers present? In configure.in the "magic" if statement (hunk: @@ -3013,6 +3066,8 @@) has a specific order from fastest to slowest. Can you please point me/us at any reliable /dev/poll benchmarking comparisons with epoll/kqueue to justify placing it where you did? Thank you. > > Note that while porting comm_devpoll.cc I have suspicions that the > comm_epoll.cc commResetSelect() function does nothing (i.e., it calls > commSetSelect with no type flag, which I suspect is a no-op for all > intents and purposes). I haven't confirmed this, however, so haven't > raised it as a bug. Someone may want to double-check. > Hmm. Yes. It stops the event FD being polled, but does not unset the handler like all the other loops. I've not yet learned the details of epoll well, if you think doing a no-op there is incorrect please make the bug report about it. Amos
