Re: [PATCH] Solaris /dev/poll support for Squid 3 (how can I contribute)

On Wed, 13 Oct 2010 15:40:22 +0100, Peter Payne <sourceforge_at_pirosa.co.uk>
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 100000 Apache Bench queries
>>
>> patched Squid (using /dev/poll):
>> 0min22sec CPU user-time
>> 25sec to process 100000 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
Received on Wed Oct 13 2010 - 22:26:04 MDT