You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "William A. Rowe, Jr." <wr...@covalent.net> on 2002/06/12 18:57:09 UTC
[patch] reduce conversions of apr_time_t
Attached is a patch to store timeout and keep_alive_timeout as apr_time_t
[whatever representation in use] as they are generally food for apr. Only
two
conversions to seconds, one in creating the Keep-Alive header, and one
in mod_info are actually done at request time. The burden of converting
to apr_time_t is moved to config-time.
I need a real +1 to proceed [e.g. grep for ->timeout
and ->keep_alive_timeout
and see that you agree I caught all the references.] This aught to be
straightened
up before we experiment with binary usec representations of apr_time_t.
Bill
Re: [patch] reduce conversions of apr_time_t
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 01:29 PM 6/12/2002, Cliff Woolley wrote:
>On Wed, 12 Jun 2002, William A. Rowe, Jr. wrote:
>
> > I need a real +1 to proceed [e.g. grep for ->timeout and
> > ->keep_alive_timeout and see that you agree I caught all the
> > references.] This aught to be straightened up before we experiment with
> > binary usec representations of apr_time_t.
>
>+1 with the following comments:
>
>First of all, why apr_time_t (or its eventual equivalent)? This should be
>an apr_interval_time_t (or its eventual equivalent). apr_time_t is *only*
>for time relative to the epoch.
Good suggestion, adopted.
> > /** Seconds we'll wait for another request */
>
>This comment needs to change as well.
Done.
> > --- modules/generators/mod_info.c 7 May 2002 23:41:36 -0000 1.47
> > +++ modules/generators/mod_info.c 12 Jun 2002 16:40:30 -0000
> > @@ -393,9 +393,10 @@
> > "<tt>%s:%u</tt></dt>\n",
> > ap_get_server_name(r), ap_get_server_port(r));
> > ap_rprintf(r, "<dt><strong>Timeouts:</strong> "
> > - "<tt>connection: %d "
> > - "keep-alive: %d</tt></dt>",
> > - serv->timeout, serv->keep_alive_timeout);
> > + "<tt>connection: %.2f "
> > + "keep-alive: %.2f</tt></dt>",
> > + (double)serv->timeout / APR_USEC_PER_SEC,
> > + (double)serv->keep_alive_timeout /
> APR_USEC_PER_SEC);
>
>I don't think this is right. The timeouts are still seconds, they're just
>represented in apr_time_t (with 0 microseconds always). You should keep
>it as %d and use APR_TIME_SEC(serv->timeout) and so on.
Well, that's stolen from mod_status. Actually, timeouts in APR can be whatever
resolution the platform supports. That said;
> > - cmd->server->keep_alive_timeout = atoi(arg);
> > + cmd->server->keep_alive_timeout = APR_TIME_FROM_SEC(atoi(arg));
>
>(This being the reason for that.)
Of course I recognized this, too. We don't [in config syntax] support timeouts
with any granularity other than seconds. That doesn't affect how they are
used,
but it could affect how they are displayed.
I'm fine with the suggestion, went to an (int) cast in the mod_info patch :-)
> > + apr_psprintf(r->pool, "timeout=%d, max=%d",
> >
> + (int)APR_TIME_SEC(r->server->keep_alive_timeout),
> > + left));
>
>Why the cast to int? Oh, because APR_TIME_SEC() returns a 64 bit
>quantity. This is what made me realize the apr_time_t vs.
>apr_interval_time_t thing. ...
Exactly. It's been observed on list that seconds shouldn't be restricted to
64 bits, so the cast allows us to deliberately toss the resolution.
>You seem to have forgotten this:
>
>Index: config.c
>===================================================================
>RCS file: /home/cvs/httpd-2.0/server/config.c,v
>retrieving revision 1.151
>diff -u -d -r1.151 config.c
>--- config.c 20 May 2002 15:05:43 -0000 1.151
>+++ config.c 12 Jun 2002 18:31:43 -0000
>@@ -1726,8 +1726,8 @@
> s->limit_req_line = DEFAULT_LIMIT_REQUEST_LINE;
> s->limit_req_fieldsize = DEFAULT_LIMIT_REQUEST_FIELDSIZE;
> s->limit_req_fields = DEFAULT_LIMIT_REQUEST_FIELDS;
>- s->timeout = DEFAULT_TIMEOUT;
>- s->keep_alive_timeout = DEFAULT_KEEPALIVE_TIMEOUT;
>+ s->timeout = APR_TIME_FROM_SEC(DEFAULT_TIMEOUT);
>+ s->keep_alive_timeout = APR_TIME_FROM_SEC(DEFAULT_KEEPALIVE_TIMEOUT);
> s->keep_alive_max = DEFAULT_KEEPALIVE;
> s->keep_alive = 1;
> s->next = NULL;
>
>
>Everything else seems okay... I can't find any other uses of this.
Thanks. Revised patch to follow.
Re: [patch] reduce conversions of apr_time_t
Posted by "Roy T. Fielding" <fi...@apache.org>.
Why do that when it is more effective to just blow away apr_time_t and
use the already-portable time_t when we want to store seconds? I have
no need for microseconds outside of struct tm (which does need a more
portable apr structure type).
....Roy
Re: [patch] reduce conversions of apr_time_t
Posted by Cliff Woolley <jw...@virginia.edu>.
On Wed, 12 Jun 2002, William A. Rowe, Jr. wrote:
> I need a real +1 to proceed [e.g. grep for ->timeout and
> ->keep_alive_timeout and see that you agree I caught all the
> references.] This aught to be straightened up before we experiment with
> binary usec representations of apr_time_t.
+1 with the following comments:
First of all, why apr_time_t (or its eventual equivalent)? This should be
an apr_interval_time_t (or its eventual equivalent). apr_time_t is *only*
for time relative to the epoch.
Give or take that little issue, here are some inline comments:
> Index: include/httpd.h
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/include/httpd.h,v
> retrieving revision 1.184
> diff -u -r1.184 httpd.h
> --- include/httpd.h 30 May 2002 07:04:45 -0000 1.184
> +++ include/httpd.h 12 Jun 2002 16:40:29 -0000
> @@ -1078,10 +1078,10 @@
>
> /** I haven't got a clue */
Oh THAT'S nice. ;) Not that it has anything to do with this patch, of
course.
> server_addr_rec *addrs;
> - /** Timeout, in seconds, before we give up */
> - int timeout;
> + /** Timeout, in apr time, before we give up */
> + apr_time_t timeout;
> /** Seconds we'll wait for another request */
This comment needs to change as well.
> Index: modules/generators/mod_info.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/generators/mod_info.c,v
> retrieving revision 1.47
> diff -u -r1.47 mod_info.c
> --- modules/generators/mod_info.c 7 May 2002 23:41:36 -0000 1.47
> +++ modules/generators/mod_info.c 12 Jun 2002 16:40:30 -0000
> @@ -393,9 +393,10 @@
> "<tt>%s:%u</tt></dt>\n",
> ap_get_server_name(r), ap_get_server_port(r));
> ap_rprintf(r, "<dt><strong>Timeouts:</strong> "
> - "<tt>connection: %d "
> - "keep-alive: %d</tt></dt>",
> - serv->timeout, serv->keep_alive_timeout);
> + "<tt>connection: %.2f "
> + "keep-alive: %.2f</tt></dt>",
> + (double)serv->timeout / APR_USEC_PER_SEC,
> + (double)serv->keep_alive_timeout / APR_USEC_PER_SEC);
> ap_mpm_query(AP_MPMQ_MAX_DAEMON_USED, &max_daemons);
> ap_mpm_query(AP_MPMQ_IS_THREADED, &threaded);
> ap_mpm_query(AP_MPMQ_IS_FORKED, &forked);
I don't think this is right. The timeouts are still seconds, they're just
represented in apr_time_t (with 0 microseconds always). You should keep
it as %d and use APR_TIME_SEC(serv->timeout) and so on.
> - cmd->server->keep_alive_timeout = atoi(arg);
> + cmd->server->keep_alive_timeout = APR_TIME_FROM_SEC(atoi(arg));
(This being the reason for that.)
> Index: modules/http/http_protocol.c
> ===================================================================
> ...
> + apr_psprintf(r->pool, "timeout=%d, max=%d",
> + (int)APR_TIME_SEC(r->server->keep_alive_timeout),
> + left));
Why the cast to int? Oh, because APR_TIME_SEC() returns a 64 bit
quantity. This is what made me realize the apr_time_t vs.
apr_interval_time_t thing. ...
You seem to have forgotten this:
Index: config.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/config.c,v
retrieving revision 1.151
diff -u -d -r1.151 config.c
--- config.c 20 May 2002 15:05:43 -0000 1.151
+++ config.c 12 Jun 2002 18:31:43 -0000
@@ -1726,8 +1726,8 @@
s->limit_req_line = DEFAULT_LIMIT_REQUEST_LINE;
s->limit_req_fieldsize = DEFAULT_LIMIT_REQUEST_FIELDSIZE;
s->limit_req_fields = DEFAULT_LIMIT_REQUEST_FIELDS;
- s->timeout = DEFAULT_TIMEOUT;
- s->keep_alive_timeout = DEFAULT_KEEPALIVE_TIMEOUT;
+ s->timeout = APR_TIME_FROM_SEC(DEFAULT_TIMEOUT);
+ s->keep_alive_timeout = APR_TIME_FROM_SEC(DEFAULT_KEEPALIVE_TIMEOUT);
s->keep_alive_max = DEFAULT_KEEPALIVE;
s->keep_alive = 1;
s->next = NULL;
Everything else seems okay... I can't find any other uses of this.
--Cliff