On Tue, Oct 6, 2015 at 12:15 PM, Graham Leggett <[email protected]> wrote: > 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: [] >> >> 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 pointer. 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)... Regards, Yann.
