You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Stas Bekman <st...@stason.org> on 2002/03/14 12:02:46 UTC
[PATCH modules/generators/mod_status.c] performance
this patch:
- removes if (!short_report) { } condition since the code inside this
condition is already in the block of the same condition.
+ fixed alignment after removing the loop
- moves a chunk of code to the condition's:
if (ws_record.access_count != 0 ||
( ws_record.status != SERVER_READY
&& ws_record.status != SERVER_DEAD)) {
block, since it was just wasting CPU cycles. Especially if a server has
high server|thread_limit but has just a few threads/procs running.
Since I realize that with fixed alignment it's harder to understand the
change I attach the change without alignment followed by the same patch
properly aligned (i've removed the diff header, so you won't commit it
this way by mistake :) normal patch follows)
Index: modules/generators/mod_status.c
@@ -556,6 +556,11 @@
for (i = 0; i < server_limit; ++i) {
for (j = 0; j < thread_limit; ++j) {
ws_record = ap_scoreboard_image->servers[i][j];
+
+ if (ws_record.access_count != 0 ||
+ ( ws_record.status != SERVER_READY
+ && ws_record.status != SERVER_DEAD)) {
+
ps_record = ap_scoreboard_image->parent[i];
if (ws_record.start_time == 0L)
@@ -573,9 +578,6 @@
my_bytes = ws_record.my_bytes_served;
conn_bytes = ws_record.conn_bytes;
- if (lres != 0 || (ws_record.status != SERVER_READY
- && ws_record.status != SERVER_DEAD)) {
- if (!short_report) {
if (no_table_report) {
if (ws_record.status == SERVER_DEAD)
ap_rprintf(r,
@@ -742,7 +744,6 @@
ap_escape_html(r->pool,
ws_record.request));
} /* no_table_report */
- } /* !short_report */
} /* if (<active child>) */
} /* for () */
}
This is the real patch:
Index: modules/generators/mod_status.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/modules/generators/mod_status.c,v
retrieving revision 1.57
diff -u -r1.57 mod_status.c
--- modules/generators/mod_status.c 13 Mar 2002 20:47:48 -0000 1.57
+++ modules/generators/mod_status.c 14 Mar 2002 10:48:03 -0000
@@ -556,193 +556,194 @@
for (i = 0; i < server_limit; ++i) {
for (j = 0; j < thread_limit; ++j) {
ws_record = ap_scoreboard_image->servers[i][j];
- ps_record = ap_scoreboard_image->parent[i];
- if (ws_record.start_time == 0L)
- req_time = 0L;
- else
- req_time = (long)
- ((ws_record.stop_time - ws_record.start_time) / 1000);
- if (req_time < 0L)
- req_time = 0L;
-
- lres = ws_record.access_count;
- my_lres = ws_record.my_access_count;
- conn_lres = ws_record.conn_count;
- bytes = ws_record.bytes_served;
- my_bytes = ws_record.my_bytes_served;
- conn_bytes = ws_record.conn_bytes;
-
- if (lres != 0 || (ws_record.status != SERVER_READY
- && ws_record.status != SERVER_DEAD)) {
- if (!short_report) {
- if (no_table_report) {
- if (ws_record.status == SERVER_DEAD)
- ap_rprintf(r,
- "<b>Server %d-%d</b> (-): %d|%lu|%lu [",
- i, (int)ps_record.generation,
+ if (ws_record.access_count != 0 ||
+ ( ws_record.status != SERVER_READY
+ && ws_record.status != SERVER_DEAD)) {
+
+ ps_record = ap_scoreboard_image->parent[i];
+
+ if (ws_record.start_time == 0L)
+ req_time = 0L;
+ else
+ req_time = (long)
+ ((ws_record.stop_time - ws_record.start_time) / 1000);
+ if (req_time < 0L)
+ req_time = 0L;
+
+ lres = ws_record.access_count;
+ my_lres = ws_record.my_access_count;
+ conn_lres = ws_record.conn_count;
+ bytes = ws_record.bytes_served;
+ my_bytes = ws_record.my_bytes_served;
+ conn_bytes = ws_record.conn_bytes;
+
+ if (no_table_report) {
+ if (ws_record.status == SERVER_DEAD)
+ ap_rprintf(r,
+ "<b>Server %d-%d</b> (-): %d|%lu|%lu [",
+ i, (int)ps_record.generation,
+ (int)conn_lres, my_lres, lres);
+ else
+ ap_rprintf(r,
+ "<b>Server %d-%d</b> (%"
+ APR_OS_PROC_T_FMT "): %d|%lu|%lu [",
+ i, (int) ps_record.generation,
+ ps_record.pid,
(int)conn_lres, my_lres, lres);
- else
- ap_rprintf(r,
- "<b>Server %d-%d</b> (%"
- APR_OS_PROC_T_FMT "): %d|%lu|%lu [",
- i, (int) ps_record.generation,
- ps_record.pid,
- (int)conn_lres, my_lres, lres);
-
- switch (ws_record.status) {
- case SERVER_READY:
- ap_rputs("Ready", r);
- break;
- case SERVER_STARTING:
- ap_rputs("Starting", r);
- break;
- case SERVER_BUSY_READ:
- ap_rputs("<b>Read</b>", r);
- break;
- case SERVER_BUSY_WRITE:
- ap_rputs("<b>Write</b>", r);
- break;
- case SERVER_BUSY_KEEPALIVE:
- ap_rputs("<b>Keepalive</b>", r);
- break;
- case SERVER_BUSY_LOG:
- ap_rputs("<b>Logging</b>", r);
- break;
- case SERVER_BUSY_DNS:
- ap_rputs("<b>DNS lookup</b>", r);
- break;
- case SERVER_CLOSING:
- ap_rputs("<b>Closing</b>", r);
- break;
- case SERVER_DEAD:
- ap_rputs("Dead", r);
- break;
- case SERVER_GRACEFUL:
- ap_rputs("Graceful", r);
- break;
- case SERVER_IDLE_KILL:
- ap_rputs("Dying", r);
- break;
- default:
- ap_rputs("?STATE?", r);
- break;
- }
+
+ switch (ws_record.status) {
+ case SERVER_READY:
+ ap_rputs("Ready", r);
+ break;
+ case SERVER_STARTING:
+ ap_rputs("Starting", r);
+ break;
+ case SERVER_BUSY_READ:
+ ap_rputs("<b>Read</b>", r);
+ break;
+ case SERVER_BUSY_WRITE:
+ ap_rputs("<b>Write</b>", r);
+ break;
+ case SERVER_BUSY_KEEPALIVE:
+ ap_rputs("<b>Keepalive</b>", r);
+ break;
+ case SERVER_BUSY_LOG:
+ ap_rputs("<b>Logging</b>", r);
+ break;
+ case SERVER_BUSY_DNS:
+ ap_rputs("<b>DNS lookup</b>", r);
+ break;
+ case SERVER_CLOSING:
+ ap_rputs("<b>Closing</b>", r);
+ break;
+ case SERVER_DEAD:
+ ap_rputs("Dead", r);
+ break;
+ case SERVER_GRACEFUL:
+ ap_rputs("Graceful", r);
+ break;
+ case SERVER_IDLE_KILL:
+ ap_rputs("Dying", r);
+ break;
+ default:
+ ap_rputs("?STATE?", r);
+ break;
+ }
#ifndef HAVE_TIMES
- /* Allow for OS/2 not having CPU stats */
- ap_rprintf(r, "]\n %.0f %ld (",
+ /* Allow for OS/2 not having CPU stats */
+ ap_rprintf(r, "]\n %.0f %ld (",
#else
- ap_rprintf(r, "] u%g s%g cu%g cs%g\n %ld %ld (",
- ws_record.times.tms_utime / tick,
- ws_record.times.tms_stime / tick,
- ws_record.times.tms_cutime / tick,
- ws_record.times.tms_cstime / tick,
+ ap_rprintf(r, "] u%g s%g cu%g cs%g\n %ld %ld (",
+ ws_record.times.tms_utime / tick,
+ ws_record.times.tms_stime / tick,
+ ws_record.times.tms_cutime / tick,
+ ws_record.times.tms_cstime / tick,
#endif
- (long)((nowtime - ws_record.last_used) /
- APR_USEC_PER_SEC),
- (long) req_time);
-
- format_byte_out(r, conn_bytes);
- ap_rputs("|", r);
- format_byte_out(r, my_bytes);
- ap_rputs("|", r);
- format_byte_out(r, bytes);
- ap_rputs(")\n", r);
+ (long)((nowtime - ws_record.last_used) /
+ APR_USEC_PER_SEC),
+ (long) req_time);
+
+ format_byte_out(r, conn_bytes);
+ ap_rputs("|", r);
+ format_byte_out(r, my_bytes);
+ ap_rputs("|", r);
+ format_byte_out(r, bytes);
+ ap_rputs(")\n", r);
+ ap_rprintf(r,
+ " <i>%s {%s}</i> <b>[%s]</b><br />\n\n",
+ ap_escape_html(r->pool,
+ ws_record.client),
+ ap_escape_html(r->pool,
+ ws_record.request),
+ ap_escape_html(r->pool,
+ ws_record.vhost));
+ }
+ else { /* !no_table_report */
+ if (ws_record.status == SERVER_DEAD)
ap_rprintf(r,
- " <i>%s {%s}</i> <b>[%s]</b><br />\n\n",
- ap_escape_html(r->pool,
- ws_record.client),
- ap_escape_html(r->pool,
- ws_record.request),
- ap_escape_html(r->pool,
- ws_record.vhost));
+ "<tr><td><b>%d-%d</b></td><td>-</td><td>%d/%lu/%lu",
+ i, (int)ps_record.generation,
+ (int)conn_lres, my_lres, lres);
+ else
+ ap_rprintf(r,
+ "<tr><td><b>%d-%d</b></td><td>%"
+ APR_OS_PROC_T_FMT
+ "</td><td>%d/%lu/%lu",
+ i, (int)ps_record.generation,
+ ps_record.pid, (int)conn_lres,
+ my_lres, lres);
+
+ switch (ws_record.status) {
+ case SERVER_READY:
+ ap_rputs("</td><td>_", r);
+ break;
+ case SERVER_STARTING:
+ ap_rputs("</td><td><b>S</b>", r);
+ break;
+ case SERVER_BUSY_READ:
+ ap_rputs("</td><td><b>R</b>", r);
+ break;
+ case SERVER_BUSY_WRITE:
+ ap_rputs("</td><td><b>W</b>", r);
+ break;
+ case SERVER_BUSY_KEEPALIVE:
+ ap_rputs("</td><td><b>K</b>", r);
+ break;
+ case SERVER_BUSY_LOG:
+ ap_rputs("</td><td><b>L</b>", r);
+ break;
+ case SERVER_BUSY_DNS:
+ ap_rputs("</td><td><b>D</b>", r);
+ break;
+ case SERVER_CLOSING:
+ ap_rputs("</td><td><b>C</b>", r);
+ break;
+ case SERVER_DEAD:
+ ap_rputs("</td><td>.", r);
+ break;
+ case SERVER_GRACEFUL:
+ ap_rputs("</td><td>G", r);
+ break;
+ case SERVER_IDLE_KILL:
+ ap_rputs("</td><td>I", r);
+ break;
+ default:
+ ap_rputs("</td><td>?", r);
+ break;
}
- else { /* !no_table_report */
- if (ws_record.status == SERVER_DEAD)
- ap_rprintf(r,
- "<tr><td><b>%d-%d</b></td><td>-</td><td>%d/%lu/%lu",
- i, (int)ps_record.generation,
- (int)conn_lres, my_lres, lres);
- else
- ap_rprintf(r,
- "<tr><td><b>%d-%d</b></td><td>%"
- APR_OS_PROC_T_FMT
- "</td><td>%d/%lu/%lu",
- i, (int)ps_record.generation,
- ps_record.pid, (int)conn_lres,
- my_lres, lres);
-
- switch (ws_record.status) {
- case SERVER_READY:
- ap_rputs("</td><td>_", r);
- break;
- case SERVER_STARTING:
- ap_rputs("</td><td><b>S</b>", r);
- break;
- case SERVER_BUSY_READ:
- ap_rputs("</td><td><b>R</b>", r);
- break;
- case SERVER_BUSY_WRITE:
- ap_rputs("</td><td><b>W</b>", r);
- break;
- case SERVER_BUSY_KEEPALIVE:
- ap_rputs("</td><td><b>K</b>", r);
- break;
- case SERVER_BUSY_LOG:
- ap_rputs("</td><td><b>L</b>", r);
- break;
- case SERVER_BUSY_DNS:
- ap_rputs("</td><td><b>D</b>", r);
- break;
- case SERVER_CLOSING:
- ap_rputs("</td><td><b>C</b>", r);
- break;
- case SERVER_DEAD:
- ap_rputs("</td><td>.", r);
- break;
- case SERVER_GRACEFUL:
- ap_rputs("</td><td>G", r);
- break;
- case SERVER_IDLE_KILL:
- ap_rputs("</td><td>I", r);
- break;
- default:
- ap_rputs("</td><td>?", r);
- break;
- }
#ifndef HAVE_TIMES
- /* Allow for OS/2 not having CPU stats */
- ap_rprintf(r, "\n</td><td>%.0f</td><td>%ld",
+ /* Allow for OS/2 not having CPU stats */
+ ap_rprintf(r, "\n</td><td>%.0f</td><td>%ld",
#else
- ap_rprintf(r, "\n</td><td>%.2f</td><td>%ld</td><td>%ld",
- (ws_record.times.tms_utime +
- ws_record.times.tms_stime +
- ws_record.times.tms_cutime +
- ws_record.times.tms_cstime) / tick,
+ ap_rprintf(r, "\n</td><td>%.2f</td><td>%ld</td><td>%ld",
+ (ws_record.times.tms_utime +
+ ws_record.times.tms_stime +
+ ws_record.times.tms_cutime +
+ ws_record.times.tms_cstime) / tick,
#endif
- (long)((nowtime - ws_record.last_used) /
- APR_USEC_PER_SEC),
- (long)req_time);
-
- ap_rprintf(r, "</td><td>%-1.1f</td><td>%-2.2f</td><td>%-2.2f\n",
- (float)conn_bytes / KBYTE, (float) my_bytes / MBYTE,
- (float)bytes / MBYTE);
-
- if (ws_record.status == SERVER_BUSY_READ)
- ap_rprintf(r,
- "</td><td>?</td><td nowrap>?</td><td nowrap>..reading.. </td></tr>\n\n");
- else
- ap_rprintf(r,
- "</td><td>%s</td><td nowrap>%s</td><td nowrap>%s</td></tr>\n\n",
- ap_escape_html(r->pool,
- ws_record.client),
- ap_escape_html(r->pool,
- ws_record.vhost),
- ap_escape_html(r->pool,
- ws_record.request));
- } /* no_table_report */
- } /* !short_report */
+ (long)((nowtime - ws_record.last_used) /
+ APR_USEC_PER_SEC),
+ (long)req_time);
+
+ ap_rprintf(r, "</td><td>%-1.1f</td><td>%-2.2f</td><td>%-2.2f\n",
+ (float)conn_bytes / KBYTE, (float) my_bytes / MBYTE,
+ (float)bytes / MBYTE);
+
+ if (ws_record.status == SERVER_BUSY_READ)
+ ap_rprintf(r,
+ "</td><td>?</td><td nowrap>?</td><td nowrap>..reading.. </td></tr>\n\n");
+ else
+ ap_rprintf(r,
+ "</td><td>%s</td><td nowrap>%s</td><td nowrap>%s</td></tr>\n\n",
+ ap_escape_html(r->pool,
+ ws_record.client),
+ ap_escape_html(r->pool,
+ ws_record.vhost),
+ ap_escape_html(r->pool,
+ ws_record.request));
+ } /* no_table_report */
} /* if (<active child>) */
} /* for () */
}
_____________________________________________________________________
Stas Bekman JAm_pH -- Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide http://perl.apache.org/guide
mailto:stas@stason.org http://ticketmaster.com http://apacheweek.com
http://singlesheaven.com http://perl.apache.org http://perlmonth.com/
Re: [PATCH modules/generators/mod_status.c] performance
Posted by Jeff Trawick <tr...@attglobal.net>.
Stas Bekman <st...@stason.org> writes:
> But the whole code inside if(ap_extended_status) is irrelevant when
> short_report is true. If I'm not mistaken this if() condition should
> be removed including the similar if (!short_report) before the for
> loop and simply to say:
>
> if (ap_extended_status && !short_report) {
> if (no_table_report)
>
> for (i = 0; i < server_limit; ++i) {
> for (j = 0; j < thread_limit; ++j) {
> ...
> }
> }
> }
gotcha! I'll commit something soon-ish.
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
Re: [PATCH modules/generators/mod_status.c] performance
Posted by Stas Bekman <st...@stason.org>.
Jeff Trawick wrote:
> Stas Bekman <st...@stason.org> writes:
>
>
>>this patch:
>>- removes if (!short_report) { } condition since the code inside this
>> condition is already in the block of the same condition.
>>
>
> Stas, I didn't see that the short_report condition was redundant.
> Please check.
You are correct Jeff.
But the whole code inside if(ap_extended_status) is irrelevant when
short_report is true. If I'm not mistaken this if() condition should be
removed including the similar if (!short_report) before the for loop and
simply to say:
if (ap_extended_status && !short_report) {
if (no_table_report)
for (i = 0; i < server_limit; ++i) {
for (j = 0; j < thread_limit; ++j) {
...
}
}
}
Am I right? This also saves a few hundred if not thousands if() calls on
servers with many threads.
This code definitely needs some loop unrolling/simplifications, it's
extremely hard to follow the logic when the bodies of the conditions are
so big.
> Also, I guess I shouldn't try to simultaneously commit code and try to
> get my daughter to wake up and eat breakfast because I forgot to add a
> "Submitted by:" to the commit. Sorry!
Nuh, that's OK :)
_____________________________________________________________________
Stas Bekman JAm_pH -- Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide http://perl.apache.org/guide
mailto:stas@stason.org http://ticketmaster.com http://apacheweek.com
http://singlesheaven.com http://perl.apache.org http://perlmonth.com/
Re: [PATCH modules/generators/mod_status.c] performance
Posted by Jeff Trawick <tr...@attglobal.net>.
Stas Bekman <st...@stason.org> writes:
> this patch:
> - removes if (!short_report) { } condition since the code inside this
> condition is already in the block of the same condition.
Stas, I didn't see that the short_report condition was redundant.
Please check.
Also, I guess I shouldn't try to simultaneously commit code and try to
get my daughter to wake up and eat breakfast because I forgot to add a
"Submitted by:" to the commit. Sorry!
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
Re: [PATCH modules/generators/mod_status.c] performance
Posted by Stas Bekman <st...@stason.org>.
Jeff Trawick wrote:
> Stas Bekman <st...@stason.org> writes:
>
>
>>this patch:
>>- removes if (!short_report) { } condition since the code inside this
>> condition is already in the block of the same condition.
>> + fixed alignment after removing the loop
>>- moves a chunk of code to the condition's:
>>
>> if (ws_record.access_count != 0 ||
>> ( ws_record.status != SERVER_READY
>> && ws_record.status != SERVER_DEAD)) {
>>
>>block, since it was just wasting CPU cycles. Especially if a server has
>>high server|thread_limit but has just a few threads/procs running.
>>
>
> maybe since the body of the loop is so darn big it would be clearer if
> the check was:
>
> + if (ws_record.access_count == 0 &&
> + ( ws_record.status == SERVER_READY
> + || ws_record.status == SERVER_DEAD)) {
> + continue;
> + }
yes! I hate multiply nested loops with huge bodies. Go for it Jeff.
--
_____________________________________________________________________
Stas Bekman JAm_pH -- Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide http://perl.apache.org/guide
mailto:stas@stason.org http://ticketmaster.com http://apacheweek.com
http://singlesheaven.com http://perl.apache.org http://perlmonth.com/
Re: [PATCH modules/generators/mod_status.c] performance
Posted by Jeff Trawick <tr...@attglobal.net>.
Stas Bekman <st...@stason.org> writes:
> this patch:
> - removes if (!short_report) { } condition since the code inside this
> condition is already in the block of the same condition.
> + fixed alignment after removing the loop
> - moves a chunk of code to the condition's:
>
> if (ws_record.access_count != 0 ||
> ( ws_record.status != SERVER_READY
> && ws_record.status != SERVER_DEAD)) {
>
> block, since it was just wasting CPU cycles. Especially if a server has
> high server|thread_limit but has just a few threads/procs running.
maybe since the body of the loop is so darn big it would be clearer if
the check was:
+ if (ws_record.access_count == 0 &&
+ ( ws_record.status == SERVER_READY
+ || ws_record.status == SERVER_DEAD)) {
+ continue;
+ }
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...