Anyone time for remote eyes if my findings are correct or wrong? Regards
Rüdiger > -----Original Message----- > From: Ruediger Pluem > Sent: Mittwoch, 28. September 2011 08:29 > To: dev@httpd.apache.org > Subject: Re: svn commit: r1176019 - in /httpd/httpd/trunk: > CHANGES modules/filters/mod_substitute.c > > > > On 09/26/2011 10:09 PM, s...@apache.org wrote: > > Author: sf > > Date: Mon Sep 26 20:09:06 2011 > > New Revision: 1176019 > > > > URL: http://svn.apache.org/viewvc?rev=1176019&view=rev > > Log: > > Make mod_substitute more efficient: > > - Use varbuf resizable buffer instead of constantly allocating pool > > memory and copying data around. This changes the memory > requirement from > > quadratic in ((number of substitutions in line) * (length > of line)) to > > linear in (length of line). > > - Instead of copying buckets just to append a \0, use new > ap_regexec_len() > > function > > > > PR: 50559 > > > > Modified: > > httpd/httpd/trunk/CHANGES > > httpd/httpd/trunk/modules/filters/mod_substitute.c > > > > Modified: httpd/httpd/trunk/modules/filters/mod_substitute.c > > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters > /mod_substitute.c?rev=1176019&r1=1176018&r2=1176019&view=diff > > > ============================================================== > ================ > > --- httpd/httpd/trunk/modules/filters/mod_substitute.c (original) > > +++ httpd/httpd/trunk/modules/filters/mod_substitute.c Mon > Sep 26 20:09:06 2011 > /* > > @@ -189,82 +174,70 @@ static void do_pattmatch(ap_filter_t *f, > > bytes -= len; > > buff += len; > > } > > - if (script->flatten && s1 && !force_quick) { > > - /* > > - * we've finished looking at the > bucket, so remove the > > - * old one and add in our new one > > - */ > > - s2 = apr_pstrmemdup(tmp_pool, buff, bytes); > > - s1 = apr_pstrcat(tmp_pool, s1, s2, NULL); > > - tmp_b = > apr_bucket_transient_create(s1, strlen(s1), > > - > f->r->connection->bucket_alloc); > > + if (have_match && script->flatten && > !force_quick) { > > + char *copy = ap_varbuf_pdup(pool, > &vb, NULL, 0, > > + buff, > bytes, &len); > > + tmp_b = > apr_bucket_pool_create(copy, len, pool, > > + > f->r->connection->bucket_alloc); > > APR_BUCKET_INSERT_BEFORE(b, tmp_b); > > apr_bucket_delete(b); > > b = tmp_b; > > } > > - > > } > > else if (script->regexp) { > > - /* > > - * we need a null terminated string > here :(. To hopefully > > - * save time and memory, we don't > alloc for each run > > - * through, but only if we need to > have a larger chunk > > - * to save the string to. So we keep > track of how much > > - * we've allocated and only re-alloc > when we need it. > > - * NOTE: this screams for a macro. > > - */ > > - if (!scratch || (bytes > (fbytes + 1))) { > > Not that it matters on trunk now any longer, but on 2.2.x: > > Don't we have an off by two here? > If scratch was already allocated it is fbytes large. > If bytes is e.g. equal to fbytes or bytes == fbytes +1 > the above won't be true, but we will > copy fbytes + 1 (2) data (trailing \0) to the buffer. > Shouldn't the check be > > bytes + 1 > fbytes > > instead? > > > - fbytes = bytes + 1; > > - scratch = apr_palloc(tpool, fbytes); > > - } > > - /* reset pointer to the scratch space */ > > - p = scratch; > > - memcpy(p, buff, bytes); > > - p[bytes] = '\0'; > > - while (!ap_regexec(script->regexp, p, > > + int left = bytes; > > + const char *pos = buff; > > + while (!ap_regexec_len(script->regexp, > pos, left, > > AP_MAX_REG_MATCH, > regm, 0)) { > > - /* first, grab the replacement string */ > > - repl = ap_pregsub(tmp_pool, > script->replacement, p, > > - AP_MAX_REG_MATCH, regm); > > + have_match = 1; > > if (script->flatten && !force_quick) { > > - SEDSCAT(s1, s2, tmp_pool, p, > regm[0].rm_so, repl); > > + /* copy bytes before the match */ > > + if (regm[0].rm_so > 0) > > + ap_varbuf_strmemcat(&vb, > pos, regm[0].rm_so); > > + /* add replacement string */ > > + ap_varbuf_regsub(&vb, > script->replacement, pos, > > + > AP_MAX_REG_MATCH, regm); > > } > > else { > > + repl = ap_pregsub(pool, > script->replacement, pos, > > + > AP_MAX_REG_MATCH, regm); > > len = (apr_size_t) > (regm[0].rm_eo - regm[0].rm_so); > > SEDRMPATBCKT(b, regm[0].rm_so, > tmp_b, len); > > tmp_b = > apr_bucket_transient_create(repl, > > - > strlen(repl), > > + strlen(repl), > > > f->r->connection->bucket_alloc); > > APR_BUCKET_INSERT_BEFORE(b, tmp_b); > > } > > /* > > - * reset to past what we just did. > buff now maps to b > > + * reset to past what we just did. > pos now maps to b > > * again > > */ > > - p += regm[0].rm_eo; > > + pos += regm[0].rm_eo; > > + left -= regm[0].rm_eo; > > } > > - if (script->flatten && s1 && !force_quick) { > > - s1 = apr_pstrcat(tmp_pool, s1, p, NULL); > > - tmp_b = > apr_bucket_transient_create(s1, strlen(s1), > > - > f->r->connection->bucket_alloc); > > + if (have_match && script->flatten && > !force_quick) { > > + char *copy; > > + /* Copy result plus the part after > the last match into > > + * a bucket. > > + */ > > + copy = ap_varbuf_pdup(pool, &vb, > NULL, 0, pos, left, > > + &len); > > + tmp_b = > apr_bucket_pool_create(copy, len, pool, > > + > f->r->connection->bucket_alloc); > > APR_BUCKET_INSERT_BEFORE(b, tmp_b); > > apr_bucket_delete(b); > > b = tmp_b; > > } > > - > > } > > else { > > - /* huh? */ > > + ap_assert(0); > > continue; > > } > > } > > } > > script++; > > } > > - > > - apr_pool_destroy(tpool); > > - > > - return; > > + ap_varbuf_free(&vb); > > } > > > > static apr_status_t substitute_filter(ap_filter_t *f, > apr_bucket_brigade *bb) > > > > > > > > > > > Regards > > Rüdiger >
