On 06 Oct 2015, at 1:39 AM, Yann Ylavic <[email protected]> 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:
>
> # util_filter.h:
> struct ap_filter_data;
> struct ap_filter_t {
> ...
> struct ap_filter_data *data;
> };
>
> # util_filter.c:
> struct ap_filter_data
> {
> /** Buffered data associated with the current filter. */
> apr_bucket_brigade *bb;
>
> /** Dedicated pool to use for deferred writes. */
> apr_pool_t *deferred_pool;
> };
>
> 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.
I would rather finish the filter suspension work before doing any optimising
like this, it’s too soon at this stage.
> 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.
Fortunately this loop only really does anything at the bottleneck point in the
filter stack, which is typically the first filter that does any meaningful
work. After this point buckets come down the filter stack one bucket at a time,
and the loop becomes one big if statement for subsequent filters.
> This pattern looks shared by all (relevant) MPMs, couldn't we make it
> a function? Maybe ap_reinstate_conn()?
Eventually yes.
For reasons I wasn’t sure of, each MPM was subtly different to the next in this
loop. I want to first investigate why they work differently before baking this
behaviour in.
I also want to solve the filter suspend problem before trying to optimise like
this.
> 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.
> Couldn't we use a (single) linked list for c->filters since
> ap_filter_t already has a 'next' field?
We can’t alas, as c->filters is not a linked list but rather a set.
Most specifically, it is the set of filters that have set aside data. Setting
aside data is optional, some filters will elect not to do so, and synchronous
filters will definitely not do so.
Filters in this set are eligible for being kicked. If you’re not in the set,
you get left alone.
>> @@ -635,7 +653,8 @@ AP_DECLARE(apr_status_t) ap_save_brigade
>> apr_status_t rv, srv = APR_SUCCESS;
>>
>> /* If have never stored any data in the filter, then we had better
>> - * create an empty bucket brigade so that we can concat.
>> + * create an empty bucket brigade so that we can concat. Register
>> + * a cleanup to zero out the pointer if the pool is cleared.
>
> Maybe this comment belongs in ap_filter_setaside_brigade()?
Will check.
>> +static apr_status_t filters_cleanup(void *data)
>> +{
>> + ap_filter_t **key = data;
>> +
>> + apr_hash_set((*key)->c->filters, key, sizeof(ap_filter_t **), NULL);
>> +
>> + return APR_SUCCESS;
>> +}
>
> 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 pointer.
> The linked list could possibly be simpler here too (using the 'next' field).
> Above filters_cleanup() would have to start from the beginning of the
> list each time to remove the entries, though.
As described above, this is a set, not a linked list.
>> + /* decide what pool we setaside to, request pool or deferred pool?
>> */
>> + if (f->r) {
>> + pool = f->r->pool;
>> + APR_BRIGADE_CONCAT(f->bb, bb);
>> + }
>> + else {
>> + if (!f->deferred_pool) {
>> + apr_pool_create(&f->deferred_pool, f->c->pool);
>> + apr_pool_tag(f->deferred_pool, "deferred_pool");
>> + }
>> + pool = f->deferred_pool;
>> + return ap_save_brigade(f, &f->bb, &bb, pool);
>> + }
>
> Shouldn't ap_save_brigade() be moved below the "else”?
It shouldn’t no.
ap_save_brigade() performs setaside, and that is precisely what we want to
avoid for request filters. In request filters we can safely APR_BRIGADE_CONCAT
and not worry.
If you moved ap_save_brigade() the server behaviour would become synchronous
again for data that wasn’t a file.
>> + *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.
Regards,
Graham
—
