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 100000 Apache Bench queries
>
> patched Squid (using /dev/poll):
> 0min22sec CPU user-time
> 25sec to process 100000 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<sourceforge_at_pirosa.co.uk>
>> 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 "XXXX...XXXX" 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
>>
>>
>