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 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
>
>