httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <>
Subject Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/
Date Tue, 06 Oct 2015 11:02:11 GMT
On Tue, Oct 6, 2015 at 12:15 PM, Graham Leggett <> wrote:
> On 06 Oct 2015, at 1:39 AM, Yann Ylavic <> wrote:
>>> +
>>> +    /** Buffered data associated with the current filter. */
>>> +    apr_bucket_brigade *bb;
>>> +
>>> +    /** Dedicated pool to use for deferred writes. */
>>> +    apr_pool_t *deferred_pool;
>> Could we make these opaque, eg:
>> This would prevent them from being mangled anywhere (they are
>> internals to util_filter.c anyway).
> There weren’t originally internal to util_filter.c, it’s only subsequently turned
out that way. Doing that comes at a cost of a further 8 bytes per filter per request through,
which starts adding up.

8 or a few more bytes is a concern, really?

> I would rather finish the filter suspension work before doing any optimising like this,
it’s too soon at this stage.

Fair enough.

>> It could also possibly avoid walking the brigade for every
>> ap_filter_reinstate_brigade() call…
> Unfortunately you can’t avoid walking the brigade in the ap_filter_reinstate_brigade(),
as this is the flow control logic that keeps the server safe from consuming too much data.

Well, there are only invariants here, the loop always computes the
same values for the buffered_bb or am I missing something?

>> Also it seems that we leak the hash iterator here (on c->pool).
> We don’t, when you create an iterator with a NULL pool it uses an iterator internal
to the hash. If we passed c->pool to apr_hash_first(), we would leak.

Correct, Rüdiger spotted it already, my bad.

>> Shouldn't we call filters_cleanup() from ap_remove_output_filter() too?
> I deliberately decided not to.
> The majority of calls to ap_remove_output_filter() are made when filters step out the
way before data starts flowing, and if we tried to run the cleanup we’d be performing an
unnecessary apr_hash_set() on every invocation before the filter was ever added to the hash.
> The point at which it would make a difference is after the EOS bucket is handled, but
at this point it doesn’t matter, all we’re doing is saving one NULL check in the MPM on
a loop with a handful of iterations, it’s not worth the expense.

>> Why not simply use:
>>   key = apr_pmemdup(pool, &f, sizeof f);
>>   apr_hash_set(f->c->filters, &key, sizeof key, f)
>> here, and:
>>   ap_filter_t *f = data;
>>   apr_hash_set(f->c->filters, &f, sizeof f, NULL);
>> in filters_cleanup() above?
> We could but it’s more expensive. The memdup part is replaced by simply assigning a

Not sure about the overhead, memcpy is optimized for small sizes anyway.
At least both "sizeof(ap_filter_t **)" here and in filters_cleanup()
should be replaced by "sizeof(ap_filter_t *)" which is syntactically
more correct (even if both are equal in C).

>>> +    *flush_upto = NULL;
>>> +
>>> +    bytes_in_brigade = 0;
>>> +    non_file_bytes_in_brigade = 0;
>>> +    eor_buckets_in_brigade = 0;
>>> +    morphing_bucket_in_brigade = 0;
>> Per the ealier suggestion to make ap_filter_data opaque, these could
>> be part of the struct (and reentrant).
>> We could then PREPEND after the loop below, and avoid potentially to
>> walk the same buckets each time.
> Alas as described above that’s not possible, as it breaks the flow control mechanism
that prevents the server generating too much data.

Unless what's in buffered_bb is opaque (immutable between calls)...


View raw message