You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by ge...@everythingsucks.co.uk on 2006/01/02 21:43:54 UTC
[Patch] poll/unix/poll.c:apr_pollset_poll() optimisation
Greetings,
Spotted an early-exit optimisation in poll/unix/poll.c:apr_pollset_poll().
We know the number of signalled descriptors, NUM, returned from the
poll() call so we could bust out of the post-processing loop once we've
added NUM signalled descriptors to our RESULT_SET.
Cheers
Gerry Calderhead
[gerry@devbox unix]$ rcsdiff -u6 poll.c
===================================================================
RCS file: RCS/poll.c,v
retrieving revision 1.1
diff -u6 -r1.1 poll.c
--- poll.c 2006/01/02 18:30:27 1.1
+++ poll.c 2006/01/02 20:16:14
@@ -259,13 +259,13 @@
j = 0;
for (i = 0; i < pollset->nelts; i++) {
if (pollset->pollset[i].revents != 0) {
pollset->result_set[j] = pollset->query_set[i];
pollset->result_set[j].rtnevents =
get_revent(pollset->pollset[i].revents);
- j++;
+ if ((++j)==(*num)) break;
}
}
if (descriptors)
*descriptors = pollset->result_set;
return APR_SUCCESS;
}
Re: UPDATE: [Patch] poll/unix/poll.c:apr_pollset_poll()
optimisation
Posted by ge...@everythingsucks.co.uk.
Quoting "Roy T. Fielding" <fi...@gbiv.com>:
> On Jan 4, 2006, at 4:42 PM, gerry@everythingsucks.co.uk wrote:
>
>> Quoting Garrett Rooney <ro...@electricjellyfish.net>:
>>
>>> Um, I don't think that's going to work, rv is the number of
>>> descriptors that hit, there's nothing that says that if N hit it'll be
>>> the first N...
>>
>> That would be true were I comparing I<RV but I'm comapring J<RV :D
>
>>>> - for (i = 0; i < pollset->nelts; i++) {
>>>> + for (i = 0; j < rv; i++) {
>
> I have a vague recollection that there was some reason we couldn't
> trust the return value being equal to the number of ready descriptors
> on some platform, but I have no time to search the archives for why.
> I may be remembering something related to ... er .. that other call
> like poll... hmmm, oh, select. Or maybe I just need more caffeine.
>
> ....Roy
>
You are correct, select() returns the total number of descriptors in
the FDSET rather than those which have had events.
However the patch applies specifically to the poll() implementation for
pollset, select() remains unchanged for that very reason.
Re: UPDATE: [Patch] poll/unix/poll.c:apr_pollset_poll() optimisation
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On Jan 4, 2006, at 4:42 PM, gerry@everythingsucks.co.uk wrote:
> Quoting Garrett Rooney <ro...@electricjellyfish.net>:
>
>> Um, I don't think that's going to work, rv is the number of
>> descriptors that hit, there's nothing that says that if N hit
>> it'll be
>> the first N...
>
> That would be true were I comparing I<RV but I'm comapring J<RV :D
>>> - for (i = 0; i < pollset->nelts; i++) {
>>> + for (i = 0; j < rv; i++) {
I have a vague recollection that there was some reason we couldn't
trust the return value being equal to the number of ready descriptors
on some platform, but I have no time to search the archives for why.
I may be remembering something related to ... er .. that other call
like poll... hmmm, oh, select. Or maybe I just need more caffeine.
....Roy
Re: UPDATE: [Patch] poll/unix/poll.c:apr_pollset_poll()
optimisation
Posted by ge...@everythingsucks.co.uk.
Quoting Garrett Rooney <ro...@electricjellyfish.net>:
> Um, I don't think that's going to work, rv is the number of
> descriptors that hit, there's nothing that says that if N hit it'll be
> the first N...
That would be true were I comparing I<RV but I'm comapring J<RV :D
> As for the other version of this patch, I'm kind of curious if it's an
> actual improvement. Sure, you can bail early, but at the cost of
> intruducing an extra if into each of the iterations before you bail.
> Is that actually better in the average case?
Yeah I kinda thought the same thing, was bugging me which is why I
revisited it.
G
> On 1/4/06, gerry@everythingsucks.co.uk <ge...@everythingsucks.co.uk> wrote:
>> Greetings,
>>
>> There's a sneakier (and pretty obvious, once you look at it) way of
>> improving this loop which I prefer to my original mail by replacing the
>> for-loop condition.
>>
>> Should be safe: I like poll()... he has an honest face... I don't think
>> he will lie to us about the number of signalled descriptors.
>>
>>
>> --- poll.c 2006/01/02 18:30:27 1.1
>> +++ poll.c 2006/01/05 00:11:36
>> @@ -254,13 +254,13 @@
>> return apr_get_netos_error();
>> }
>> if (rv == 0) {
>> return APR_TIMEUP;
>> }
>> j = 0;
>> - for (i = 0; i < pollset->nelts; i++) {
>> + for (i = 0; j < rv; i++) {
>> if (pollset->pollset[i].revents != 0) {
>> pollset->result_set[j] = pollset->query_set[i];
>> pollset->result_set[j].rtnevents =
>> get_revent(pollset->pollset[i].revents);
>> j++;
>> }
>>
>
> Um, I don't think that's going to work, rv is the number of
> descriptors that hit, there's nothing that says that if N hit it'll be
> the first N...
>
> As for the other version of this patch, I'm kind of curious if it's an
> actual improvement. Sure, you can bail early, but at the cost of
> intruducing an extra if into each of the iterations before you bail.
> Is that actually better in the average case?
>
> -garrett
>
Re: UPDATE: [Patch] poll/unix/poll.c:apr_pollset_poll() optimisation
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/4/06, gerry@everythingsucks.co.uk <ge...@everythingsucks.co.uk> wrote:
> Greetings,
>
> There's a sneakier (and pretty obvious, once you look at it) way of
> improving this loop which I prefer to my original mail by replacing the
> for-loop condition.
>
> Should be safe: I like poll()... he has an honest face... I don't think
> he will lie to us about the number of signalled descriptors.
>
>
> --- poll.c 2006/01/02 18:30:27 1.1
> +++ poll.c 2006/01/05 00:11:36
> @@ -254,13 +254,13 @@
> return apr_get_netos_error();
> }
> if (rv == 0) {
> return APR_TIMEUP;
> }
> j = 0;
> - for (i = 0; i < pollset->nelts; i++) {
> + for (i = 0; j < rv; i++) {
> if (pollset->pollset[i].revents != 0) {
> pollset->result_set[j] = pollset->query_set[i];
> pollset->result_set[j].rtnevents =
> get_revent(pollset->pollset[i].revents);
> j++;
> }
>
Um, I don't think that's going to work, rv is the number of
descriptors that hit, there's nothing that says that if N hit it'll be
the first N...
As for the other version of this patch, I'm kind of curious if it's an
actual improvement. Sure, you can bail early, but at the cost of
intruducing an extra if into each of the iterations before you bail.
Is that actually better in the average case?
-garrett
UPDATE: [Patch] poll/unix/poll.c:apr_pollset_poll() optimisation
Posted by ge...@everythingsucks.co.uk.
Greetings,
There's a sneakier (and pretty obvious, once you look at it) way of
improving this loop which I prefer to my original mail by replacing the
for-loop condition.
Should be safe: I like poll()... he has an honest face... I don't think
he will lie to us about the number of signalled descriptors.
--- poll.c 2006/01/02 18:30:27 1.1
+++ poll.c 2006/01/05 00:11:36
@@ -254,13 +254,13 @@
return apr_get_netos_error();
}
if (rv == 0) {
return APR_TIMEUP;
}
j = 0;
- for (i = 0; i < pollset->nelts; i++) {
+ for (i = 0; j < rv; i++) {
if (pollset->pollset[i].revents != 0) {
pollset->result_set[j] = pollset->query_set[i];
pollset->result_set[j].rtnevents =
get_revent(pollset->pollset[i].revents);
j++;
}
Cheers
Gerry Calderhead
Quoting gerry@everythingsucks.co.uk:
> Greetings,
>
> Spotted an early-exit optimisation in poll/unix/poll.c:apr_pollset_poll().
>
> We know the number of signalled descriptors, NUM, returned from the
> poll() call so we could bust out of the post-processing loop once
> we've added NUM signalled descriptors to our RESULT_SET.
>
> Cheers
> Gerry Calderhead
>
> [gerry@devbox unix]$ rcsdiff -u6 poll.c
> ===================================================================
> RCS file: RCS/poll.c,v
> retrieving revision 1.1
> diff -u6 -r1.1 poll.c
> --- poll.c 2006/01/02 18:30:27 1.1
> +++ poll.c 2006/01/02 20:16:14
> @@ -259,13 +259,13 @@
> j = 0;
> for (i = 0; i < pollset->nelts; i++) {
> if (pollset->pollset[i].revents != 0) {
> pollset->result_set[j] = pollset->query_set[i];
> pollset->result_set[j].rtnevents =
> get_revent(pollset->pollset[i].revents);
> - j++;
> + if ((++j)==(*num)) break;
> }
> }
> if (descriptors)
> *descriptors = pollset->result_set;
> return APR_SUCCESS;
> }
>
>